Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement named variables. #674

Merged
merged 6 commits into from
Sep 11, 2023

Conversation

kareltucek
Copy link
Collaborator

@kareltucek kareltucek commented Aug 30, 2023

Changes:

  • Introduced named, dynamically typed variables. I.e., setVar command and $ syntax. E.g.:
    setVar myDelay 50
    set doubletapDelay $myDelay
    
  • Introduced expression parser that allows in-line arithmetics (+, -, *, /, %, min(...), max(...)) and boolean conditions (<, <=, >, >=, ==, !=, !, &&, ||). E.g.:
    ifShift set leds.brightness ($leds.brightness * 1.5 + 0.01)
    ifNotShift set leds.brightness ($leds.brightness / 1.5)
    
  • Introduced if (EXPRESSION) command. E.g.:
    if ($a < 2 || $a > 4) tapKey a
    
  • Introduced string parser. Strings should from now on be enclosed in double or single quotes. Double-quote strings support variable expansion and \ escapes. E.g.:
    write "Current keystrokeDelay is $keystrokeDelay. One plus one is $(1 + 1).\n"
    write 'This is literal "$" character. And here you have an apostrophe '"'"'.'
    
  • Introduced keyid parser. As a result, ifGesture, ifShortcut and some other commands now accept keyid abbreviations. E.g.:
    ifShortcut a b holdLayer fn
    
  • Introduced validateUserConfig command, mainly for development purposes.
  • setLedTxt now accepts numeric expressions. E.g.:
    setLedTxt 500 $keystrokeDelay
    
  • Configuration values now can be retrieved using $<name> syntax just as variables. E.g.:
    set leds.brightness ($leds.brightness * 1.5 + 0.01)
    
  • Deprecated #,% and @ syntaxies.
  • Deprecated registers. I.e., ifRegEq, ifNotRegEq, ifRegGt, ifRegLt, setReg, addReg, subReg, mulReg commands
  • Deprecated writeExpr and setStatusPart commands.
  • Refactored most of the macro codebase to a new token-parsing scheme (parser context and consume* operators. This has probably introduced some new bugs. Please, let us know on github or on our forum if you encounter any.

Closes #614, closes #620, closes #630.

Sorry for the PR length 😅. Originally it was split into multiple commits, until I realized that I had forgotten to commit new files... ...which has led me to squash the branch to prevent inconsistent states on the branch.

@mondalaci
Copy link
Member

Please bear with me until I review your (very impressive) PRs; I want to release the current firmware master with proper implementation of the md5sum feature and a related Agent version first so that the chance of users bricking their UHKs would decrease dramatically.

@likern
Copy link

likern commented Sep 5, 2023

That looks very interesting. What it lacks - high quality documentation and tutorials.
The power of macros are much more powerful then what I can get from documentation.

@mondalaci
Copy link
Member

@likern Agreed, and I think #647 will resolve the mentioned issues.

@mondalaci
Copy link
Member

@kareltucek Would you please write example commands for the points mentioned?

I'm asking because I find this PR quite overwhelming, and examples I can easily test would help a lot.

Going forward, please try to submit smaller PRs. That way, I can review them much faster without getting my brain melted :)

@kareltucek
Copy link
Collaborator Author

Would you please write example commands for the points mentioned?

Edited the leading post.

@mhantsch
Copy link
Contributor

mhantsch commented Sep 9, 2023

This is impressive; I like the new syntax.

My biggest question is: how is compatibility with existing macros kept? Or simply not, and users would have to rewrite all their macros once they install this firmware?

@kareltucek
Copy link
Collaborator Author

kareltucek commented Sep 9, 2023

In this PR, most old syntax is preserved but throws deprecation warnings accompanied by migration instructions. But yes, users are expected to rewrite their macros. I expect to remove the deprecated syntaxes in the next major release.

@mondalaci
Copy link
Member

The macro engine accepts setVar asdfasdfasdfasdfasdfasdfasdf 50, even though, I assume, the variable name is too long. I guess the engine discards the end of the name from the nth character. If so, it'd make sense to prohibit overly long names.

@kareltucek
Copy link
Collaborator Author

Good point, fixed, although the name is not too long, by far. Actually, asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasd is still valid length.

@mondalaci
Copy link
Member

I suggest changing Variable not found! myDelay to Variable not found: myDelay

if ($a < 2 || $a > 4) tapKey a throws:

Error at $onInit 1/9: Failed to parse expression, closing parenthess expected.
> 9 | if ($a < 2 || $a > 4) tapKey a
>   |                  ^
Error at $onInit 1/9: Unrecognized command: >
> 9 | if ($a < 2 || $a > 4) tapKey a
>   |                  ^

@mondalaci
Copy link
Member

write 'This is literal "$" character. And here you have an apostrophe '"'"'.' throws:

Error at $onInit 1/9: Dispatch data got stuck! Please report this. his
> 9 | write 'This is literal "$" character. And here you have an apostrophe '"'"'.'
>   |         ^

@mondalaci
Copy link
Member

setLedTxt $keystrokeDelay empties the text portion of the LED display.

@mondalaci
Copy link
Member

Am I correct that this is a slightly breaking change because single-digit keyIds don't work as they used to?

@kareltucek
Copy link
Collaborator Author

Am I correct that this is a slightly breaking change because single-digit keyIds don't work as they used to?

Yes.

@mondalaci
Copy link
Member

Then, after merging this PR, I'll bump the major smart macro version and, as a result, the major firmware version.

I'm unsure whether I should bump the major smart macro and firmware version again after you eventually remove the added deprecation warnings, but I think I shouldn't.

@kareltucek
Copy link
Collaborator Author

Well, there were similar minor violations of compatibility in the past upon which you did not raise the major number. For instance, switching leds.fadeTimeout from minutes to seconds. Given this, I don't think that you are strictly obliged to bump the major number just because of that relatively minor change in the keyid semantics.

I think it makes sense to bump the major versions just once for a feature, no matter whether you do it now, or upon removal of the deprecated stuff.

In any case, other things should be done before the next major release, so please don't release major version straight after merging.

@mondalaci
Copy link
Member

Great points! What do you want to do before bumping the major version?

@kareltucek
Copy link
Collaborator Author

@kareltucek
Copy link
Collaborator Author

setLedTxt $keystrokeDelay

This is actually missing time, e.g. setLedTxt 500 $keystrokeDelay, sorry about that.

Added error when this happens.


Other two bugs are fixed.

@mondalaci
Copy link
Member

This PR seems complete. Do you still have anything to fix? I assume you'll fix #674 (comment) in separate PRs.

@kareltucek
Copy link
Collaborator Author

This is complete unless you can find other problems. I will fix the other things in separate PRs.

@mondalaci
Copy link
Member

Please resolve the conflict, and I'll merge it.

@kareltucek
Copy link
Collaborator Author

Done.

@mondalaci mondalaci merged commit 39b5d0d into UltimateHackingKeyboard:master Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants