UltimateHackingKeyboard / firmware

Ultimate Hacking Keyboard firmware
Other
420 stars 66 forks source link

Implement named variables. #674

Closed kareltucek closed 1 year ago

kareltucek commented 1 year ago

Changes:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mondalaci commented 1 year ago

@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 commented 1 year ago

Would you please write example commands for the points mentioned?

Edited the leading post.

mhantsch commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mondalaci commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mondalaci commented 1 year ago

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

kareltucek commented 1 year ago

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

Yes.

mondalaci commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kareltucek commented 1 year ago
kareltucek commented 1 year ago

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 commented 1 year ago

This PR seems complete. Do you still have anything to fix? I assume you'll fix https://github.com/UltimateHackingKeyboard/firmware/pull/674#issuecomment-1712842661 in separate PRs.

kareltucek commented 1 year ago

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

mondalaci commented 1 year ago

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

kareltucek commented 1 year ago

Done.