Code review comment for lp:~sammyshp/flashlight-firmware/2c-turbo-config

Revision history for this message
Selene ToyKeeper (toykeeper) wrote :

Looks like we were working on the same thing at similar times.

I used a somewhat different approach though...

- retained the USE_2C_MAX_TURBO compile option, for backward compatibility and a smaller binary
- made turbo config apply to more places, not just double-click-from-ramp
- made simple and advanced modes use independent turbo configs
- added a third style: no turbo (to retain simple UI's no-turbo policy)
- put the turbo config into ramp menus instead of the misc/globals menu

It'd be nice to remove some of the older code at some point, but for now it's still available as a compile option.

I really like the tricks you did to handle counting depending on which options are compiled in, and will have to keep it in mind for cases where it's needed. I'm debating how to handle variable features in menus though, since things get confusing when the UI has different items at different menu indexes in different builds. So for now I've tried to keep the menu item number consistent, even if prior items were compiled out. On the one hand, this leaves a gap in the menu where the user can change a setting but it has no effect. On the other hand, it means the documentation can give accurate info about which item is which, even when some items are missing.

Also, you make a good point about being able to use global variables instead of preprocessor symbols sometimes... since the compiler can detect when it never gets changed, and compile it out. I'm hoping to replace the entire set of config vars with a struct though, and I'm not sure yet if the same method will work there. Will have to test if the compiler can retain the same granular detection, or if it leaves redundant vars in since the container struct gets modified. Regardless, refactoring the config vars is going to be a bit messy, and requires some deeper changes, so I'm putting it off for now.

« Back to merge proposal