gcormier / megadesk

Open-source IKEA Bekant controller board
GNU General Public License v3.0
724 stars 53 forks source link

Use either/both buttons to store and recall memory #62

Closed philwmcdonald closed 3 years ago

philwmcdonald commented 3 years ago

Image stats - with serial + minmax + both-buttons all enabled: RAM: [==== ] 38.9% (used 199 bytes from 512 bytes) Flash: [========= ] 86.6% (used 7096 bytes from 8192 bytes)

philwmcdonald commented 3 years ago

I'm all out of ideas for now :)

This is the image I'm using on my wife and my megadesks.

If I encounter any problems I'll push more updates.

tagno25 commented 3 years ago

With the recent space savings, both lines 451 and 460 should be safe to remove. https://github.com/gcormier/megadesk/blob/978a7d4f56fe436e35064ed54022e4661e693edc/Code/src/megadesk.cpp#L451 https://github.com/gcormier/megadesk/blob/978a7d4f56fe436e35064ed54022e4661e693edc/Code/src/megadesk.cpp#L460 They were only there because, I ran out of flash space if I had both serial and minmax enabled.

philwmcdonald commented 3 years ago

Cool. Latest image stats:

RAM:   [====      ]  38.9% (used 199 bytes from 512 bytes)
Flash: [========= ]  87.4% (used 7160 bytes from 8192 bytes)
philwmcdonald commented 3 years ago

Not sure how people feel about the both-buttons can store enhancement and whether it should be default functionality going forward.

If both-buttons is the only-option going forward then the minmax implementation can be further simplified so that's it's say 10presses on either the up or 10 presses on the down button for instance - simplifying MINMAX implementation too.

gcormier commented 3 years ago

Amazing work! This should take care of #20

gcormier commented 3 years ago

Looking back I can't see needing more then a handful of memory positions. Button presses over 10 could be used for configuration functions, and 2-8 could be memory positions, which if done on both buttons, actually gives 16 memory positions which is .. crazy :)

So a 10x push on DOWN might set your lower bound, and 10x push on UP set the upper bound, and save it in EEPROM?

The 16x variant toggle is now no longer required either. Other options might be

Safer move mode was suggest at one point. It would not work with both-buttons. I think the usage

I think it brings the memory in line with how a lot of commercial desks operate, likely due to lawyers.

philwmcdonald commented 3 years ago

Some comments: Childlock is already covered by the physical "key", I thought about this seriously about this until I remembered it existed. Surely the safer move mode makes the desk less useful - is there any demand for it? If not, it's better not offer it as an option, as it complicates development. I'm finding both-button operation is intuitive, and as far as I can tell itdoesn't sacrifice any existing functionality. If it's the new default option, it simplifies further features. It's your call @gcormier, cause you're selling and supporting the megadesk devices. Cutting over to new functionality is complex, it ultimately depends on what customers want and the support you can offer.

Note. We also have the option to make hold-both buttons together do something - disabling sound might be an option assuming there's demand for that. Multipress of both-buttons together is much harder to implement so that's worth avoiding.

gcormier commented 3 years ago

Doh that key! I always forget about it! :)

I agree, I far prefer the "dangerous" move. And I think the min/max heights can help for the edge cases/concerns.

I'm open to all contributions if they have preprocessor defines that let's us decide what to compile in, and I can flash the Tindie versions with a "basic" version to avoid risks of any bugs or complications. However for the two-button logic, it might litter the entire code base with conditionals and make it hard to maintain as well.

I think the setting of min/max via buttons would likely be the most desired function if I had to guess, as it's not uncommon to store stuff under our desks, or maybe have a shelf/cabinet overtop that our ever-growing monitors threaten.

You've both inspired me to dust off the logic analyzer.. I've just captured the re-calibration sequence, now I need to go through and figure out the flow :)

gcormier commented 3 years ago

recal branch is up and seems to work!