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

Correct range of LCD axis step editing #13727

Merged

Conversation

Projects
None yet
2 participants
@marcio-ao
Copy link
Contributor

commented Apr 16, 2019

  • Replaced the "float62" with a "float51" data type
    • Reduce multiplier from 100 to 10
    • The old data type's range was severely limited due to the limitation of int16_t for the encoder
  • If the scaled max and min limits exceed the value of int16_t, clamp to max size
  • Fixed bug in max computation that was further halving the effective range
  • Changed steps editing screen to use "float51" instead of "float62"
    • Old effective limits: 0.00 to 163.83
    • New effective limits: 0.0 to 3276.7

This fixes bug #13696

Corrected ultralcd data-type ranges (#13696)
- Replaced the "float62" with a "float61" data type
  - Reduce multiplier from 100 to 10
- If the scaled max and min limits exceed the value of int16_t, clamp to max size
- Fixed bug in max computation that was further halfing the effective range
- Changed steps editing screen to use "float61" instead of "float62"
  - Old effective limits: 0 to 163.83
  - New effective limits: 0 to 3276.7
@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

If changing the encoder value from 32-bit to 16-bit is leading to over-constraint then we can restore it to 32 bits. The change was speculative.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@thinkyhead: For now, the fix I proposed seems to work. With the adjustment I made, the only loss is that you only get one decimal place rather than two decimal places for adjusting step/mm. If someone really wants more precision, they can still use GCODE, so not all is lost.

So, it's your call. Changing everything LCD and encoder related to 32-bits will increase Marlin's size by quite a bit.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

At the moment, the LCD encoder is essentially using a 16-bit fixed point value, so the number of decimal places dictates the maximum value the editor can reach. As such, these are the ranges for signed values:

Format Range
nn,nnn 0 to 32,768
n,nnn.n 0 to 3,276.8
nnn.nn 0 to 327.68
nn.nnn 0 to 32.768

So, any LCD data type that purports to show six digits cannot do so using a 16-bits implementation. As far as I know, steps/mm are the only place where you would reasonably expect values in the thousands, so as long as we restrict that particular case to one decimal place, I think we may be okay.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

FWIW, it may make sense to rename float61 to float51, since it is showing five digits now...

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Okay, I renamed float61 to float51. It all makes sense now. All the data types show up to five digits, which is consistent with the use of int16_t behind the scenes.

The only peculiar thing is that a couple menu items specify a max value of 9999, e.g.:

MENU_MULTIPLIER_ITEM_EDIT_CALLBACK(float51, MSG_ESTEPS, &planner.settings.axis_steps_per_mm[E_AXIS_N(active_extruder)], 5, 9999, _planner_refresh_positioning);`

The max value here is specified as 9999, but I added code that will truncate that down to the maximum the data editor can support, which in this case would be 3,276.8. So, effectively, the min and max arguments to MENU_ITEM are now suggestions and may be further limited as needed.

I think this is an acceptable solution. If there really is a desire to support more range, I would think this should be done only on 32-bit boards.

@marcio-ao marcio-ao force-pushed the marcio-ao:pr-fix-ultralcd-ranges branch from c309f53 to d8df8a1 Apr 16, 2019

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

it may make sense to rename float61 to float51

Definitely.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Take note that the encoder value was using 32-bits until a week ago, so making it 32 bits again would only increase code to the size it used to be. I figured we probably didn't need granularity of more than 32,768 steps plus or minus, but apparently one or two things want those steps to represent 1/1000th of something, giving the smaller ranges you described. So, at the very least I think it would help to add some static_assert statements to make sure that the required granularity is available for menu items that want them. I'll look into adding static_assert to the menu edit macro(s) after this is merged.

thinkyhead added some commits Apr 16, 2019

@thinkyhead thinkyhead merged commit 866e2d4 into MarlinFirmware:bugfix-2.0.x Apr 16, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@thinkyhead thinkyhead changed the title Corrected range of step editor screens in ultralcd (#13696) Correct range of LCD axis step editing Apr 16, 2019

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

So, at the very least I think it would help to add some static_assert statements to make sure that the required granularity is available for menu items that want them. I'll look into adding static_assert to the menu edit macro(s) after this is merged.

I thought about adding a static_assert, but gave up when I didn't see a straightforward way to do it. However, it would be fairly easy to add a runtime message here:

    static void action_edit(PGM_P const pstr, type_t * const ptr, const type_t minValue, const type_t maxValue, const screenFunc_t callback=NULL, const bool live=false) {
      // Make sure minv and maxv fit within int16_t
      const int16_t minv = MAX(scale(minValue), INT_MIN),
                    maxv = MIN(scale(maxValue), INT_MAX);
      init(pstr, ptr, minv, maxv - minv, scale(*ptr) - minv, edit, callback, live);
    }

Something like:

    static void action_edit(PGM_P const pstr, type_t * const ptr, const type_t minValue, const type_t maxValue, const screenFunc_t callback=NULL, const bool live=false) {
      // Make sure minv and maxv fit within int16_t
      if(scale(minValue) < INT_MIN) SERIAL_ECHOLNPGM(...);
      if(scale(maxValue) > INT_MAX) SERIAL_ECHOLNPGM(...);
      const int16_t minv = MAX(scale(minValue), INT_MIN),
                    maxv = MIN(scale(maxValue), INT_MAX);
      init(pstr, ptr, minv, maxv - minv, scale(*ptr) - minv, edit, callback, live);
    }

But this would only show a message at runtime, so yeah, static_assert would be better if it could be done that way.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

A static assert would check the range of values and the granularity of the edit to make sure there are no more than 65535 increments in that range. Most of the time the check should pass. For example, if the range given is -2 to +2 and the granularity is 0.01 then the check passes (400 <= 65535). If the range was -100 to +100 with a granularity of 0.01 the check still passes (20000 <= 65535). Basically, anything that would fail the check would be impractical to edit without a pretty generous encoder multiplier anyway. The sanity check will tell us that the range or the granularity must be reduced to be useful.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@thinkyhead: My comment about the difficulty of doing this had more to do with figuring out the macros so they can do the appropriate "static_assert" than with the mechanism of the check itself :)

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

We can probably do something like this:

  #define _MENU_ITEM_VARIANT_P(TYPE, VARIANT, USE_MULTIPLIER, PLABEL, ...) do { \
    _skipStatic = false; \
    if (_menuLineNr == _thisItemNr) { \
      if (encoderLine == _thisItemNr && ui.use_click()) { \
        _MENU_ITEM_MULTIPLIER_CHECK(USE_MULTIPLIER); \
+       MenuItem_##TYPE ::assert ## VARIANT(__VA_ARGS__); \
        MenuItem_##TYPE ::action ## VARIANT(__VA_ARGS__); \
        if (screen_changed) return; \
      } \
      if (ui.should_draw()) \
        draw_menu_item ## VARIANT ## _ ## TYPE(encoderLine == _thisItemNr, _lcdLineNr, PLABEL, ## __VA_ARGS__); \
    } \
  ++_thisItemNr; \
}while(0)

…or perhaps the assert will need to be a macro so it only passes the constexpr values. I'll experiment with this soon.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@thinkyhead: I guess that would work. I guess the assert method call would need to be "constexpr" in order for the "static_asserts" to function? As long as all the compilers support that, I think your idea would work.

@thinkyhead

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Yeah, everything associated with a static_assert has to be constexpr or the compiler explodes. So the concept will only work if an intermediate macro is used to swallow variable arguments… and if static_assert can be called from a macro. I'm sure it will be fun to disentangle.

@marcio-ao

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

Yeah, everything associated with a static_assert has to be constexpr or the compiler explodes. So the concept will only work if an intermediate macro is used to swallow variable arguments… and if static_assert can be called from a macro. I'm sure it will be fun to disentangle.

Well, I decided to take a shot at it and I failed miserably to get the constexpr range check to work, basically because of this:

https://stackoverflow.com/questions/8626055/c11-static-assert-within-constexpr-function

After futzing around and failing to get the constexpr function to work, I tried doing more macro magic and came up with #13736

@marcio-ao marcio-ao deleted the marcio-ao:pr-fix-ultralcd-ranges branch May 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.