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

Allow setting a minimum value #140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

emptynick
Copy link

This allows you to set a minimum value for an AnalogMenuItem.

Please be aware:
When merged, all usages of AnyMenuInfo and AnalogMenuInfo need to be changed from
const PROGMEM AnyMenuInfo minfoX = { "Foo", ID, EEPROM, MAX, NO_CALLBACK };
to
const PROGMEM AnyMenuInfo minfoX = { "Foo", ID, EEPROM, MIN, MAX, NO_CALLBACK };
So this is a breaking change.

Not 100% sure that this is the right way as it would be better to not require this parameter for AnyMenuInfo but only for AnalogMenuInfo.
But this would require another cast in ValueMenuItem::setCurrentValue(...)

@davetcc
Copy link
Collaborator

davetcc commented Apr 25, 2022

I’m not really sure how this differs from the existing offset support in analog menu item. That allows values to appear both negative and positive within a range.

For example if offset were 100 and maximum was 155 then the value goes from 100 to 255. If the offset were -128 and max was 255 then the value goes from -128 to +127 on the display. To get the corrected values you either just add the offset to getCurrentValue or use one of the helper functions for numeric conversion in the docs. There’s a few examples in the analog docs: https://www.thecoderscorner.com/products/arduino-libraries/tc-menu/menu-item-types/analog-menu-item/ section ‘Examples of fixed point numbering’

@emptynick
Copy link
Author

You are correct - I actually simply used analogItem->getCurrentValue() in my renderer which doesn't take offset into account.
Instead I have to use the built-in menuValueToText() method.

Nonetheless, this PR allows a minimum value to be displayed and stored without the need to modify what is stored/retreived from EEPROM.
This, in my case, cleans up my code quite a lot as I have around 20 AnalogItems from which I need a value to be displayed and stored from 1 to 512.

@davetcc
Copy link
Collaborator

davetcc commented Apr 30, 2022

I've thought about this more, it's just not possible to merge this as it is, it will break literally everyone using tcMenu, they would need a complete round trip through designer for what is a small feature. Also, this will break designer and the code struct generators will need to be fixed up meaning designer would have to be synced with the library release, and incompatible with the previous library version, we rarely ever do this because it causes so much pain.

There are many interfaces I'd like to change in IoAbstraction and tcMenu, but between the two there are many thousands of users, we need to think of those before breaking backward compatibility. How about if we provided something like getIntValueWithOffsetApplied method on the Analog item so that you could get the actual offset value. Although arguably offset is less clear than minimum value, it is really hard to change. I'll also put in a new feature issue to simplify creating of analog items in the UI too.

Further, for EEPROM persistence, you don't need to use the tcMenu provided load and save for those analog fields, you could roll your own, it's not that hard, so that the value in EEPROM is as you want it.

@davetcc
Copy link
Collaborator

davetcc commented May 7, 2022

See #141 which makes it far easier to get the actual offset applied integer value from a menu item. For non-integer cases, there is already WholeAndFraction that represents the value as a fixed point fraction.

As per the last comment, if you don't like that the value gets stored in EEPROM without the offset applied, write your own function that first serializes these particular items manually, then calls into the usual save function to do the rest, then mirror that for loading back..

@davetcc
Copy link
Collaborator

davetcc commented Jul 5, 2022

Given this would be a breaking change for nearly all users, we really cannot merge this and suggest we close it.

You can now call getIntValueIncludingOffset on all analog menu items, this automatically applies the offset for you making working with integer analog items easier.

For saving to EEPROM, if it is really critical for you that these are saved without the offset, I recommend that you remove these from the regular save by clearing the EEPROM field, and persist these yourself with the offset already applied. Unfortunately, the current behaviour is just baked into too many designs to consider changing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants