Repository navigation
Conversation
|
The core of the code here is 3 lines which are very straightforward and basically can't fail. if ((2 == click_num) && manual_memory) return manual_memory;
else if (floor_b > floor_a) return floor_b;
else return floor_a;However, it can be implemented just by changing two bytes, swapping if (cfg.ramp_floors[1] < lvl) lvl = cfg.ramp_floors[1];
...
if (cfg.ramp_floors[1] > lvl) lvl = cfg.ramp_floors[1];Not sure something that simple needs 68 lines of unit test files added. What it does need, though, is a compile-time option to enable the feature, with the default being the old behavior. And it needs to ensure all the build targets still compile, which can be a little tricky since some older models have almost no bytes left in ROM. About testing, I've found that C code compiled for AVR doesn't behave the same way as C code compiled for x86 or ARM. It has several low-level differences which can make basic math and logic operations fail on one but not the other, and I've encountered those differences multiple times. So when the production code runs on AVR, I don't trust tests which are compiled for other architectures. If I add any automated testing for Anduril, it'll be in an AVR emulator. There's a promising pull request (#148) for this that I've been meaning to test and merge, if you'd like to see the direction the testing is likely to go in the future. |
|
Update: I think 2H behavior needs to change, since with this change both 1H and 2H utilize highest ramp floor, which is not ideal (it would require use of memorized level in order for 2H to be different, and we may not want to use a memorized level for this). |
Yes. That is why I changed 2 lines in my example above, instead of just 1 line. The issue is already solved in the "swap |
|
Thank you for the attention and review. My apologies for not putting in a note earlier about how I prepared the changes which was with a single prompt in codex with gpt-5.5. I reviewed the changes and they looked reasonable, but I had clearly not mentally switched to a mindset actually suitable for reviewing embedded C code. The other aspect of it was basically that I was playing around with my new Anduril 2 light and I thought I came up with an improvement for it, and such as things are in 2026, I figured I'd check on the level of effort involved in producing an implementation. As expected the level of effort was indeed low given the simple nature of the change, but as you can see it was highly premature. I completely agree with your assessment, and the code change content of this PR is bordering on preposterous by factoring the core logic involved here into not one but two function calls. Semantic clarity being helped could be arguable, but it is a detriment when it breaks up the pertinent logic this much, turning about 5 lines worth of logic into 20+ lines. Especially when these functions are not reused anywhere to further justify their creation. It's quite clear already that changes to logic that i might propose will be exceedingly easy to code out. This is thanks to the high quality of this codebase. So, the work left to do here will be to think it through much more thoroughly about any suggested changes. As such I am closing this silly PR, thanks for humoring me. I will also note that I am able to use 6C tactical mode to get an alternative lockout mode (which is interesting as it also borrows the lockout aux pattern setting, further communicating that anduril considers tactical mode to spiritually also be a form of lockout). That is powerful since it allows for 3 levels that are possible to set to anything we want with extreme precision. Still, tactical mode as a lockout mode isn't ideal, if only because 6C is a bit hard to enter and exit (with 5C being there as a landmine). I am not done thinking about this. Some other alternatives for the 2H logic could be: A. Take the level delta between min(floors) and max(floors), and add this level delta to max(floors).
Re: testing, i wonder how difficult it would be to set up a station that automates testing on real hardware. It sounds like a lot of work for questionable gain at first. But, there are probably, maybe, neat consequences to be had if oscilloscope traces and stuff can be had in full, with robotic repeatability, across changesets. Emulation surely has higher priority. |
2 participants
Summary
This changes Lockout Mode momentary moon behavior so 1H uses the highest configured ramp floor instead of the lowest. 2H keeps its existing behavior: it uses manual memory when configured, otherwise the highest configured ramp floor.
This lets users keep an ultra-low floor on their primary ramp, while using the other ramp style’s floor as a practical lockout moon level.
Behavior change
Before:
After:
Implementation notes
The change is intentionally small: the lockout path now calls a tiny level-selection helper, with no new config state and no changes to ramp setup, manual memory, or lockout exit behavior. The helper is static inline, so it should compile away while making the rule easy to test directly.
Docs and tests
Testing
make test