cp2004 / OctoPrint-EEPROM-Marlin

A plugin for editing the EEPROM data of Marlin Firmware, from within OctoPrint.
https://plugins.octoprint.org/plugins/eeprom_marlin
GNU Affero General Public License v3.0
35 stars 14 forks source link

[Bug] Enable/Disable value for the Auto Bed Level allows a decimal input #35

Closed KenLucke closed 3 years ago

KenLucke commented 3 years ago

Describe the bug

The Enable/Disable value for the Auto Bed Level allows a decimal input, while it should be a 1/0 only possibility. Not sure if it rounds it when storing, but even that would create some ambiguity.

Instead of a value entry with up/down arrows, I believe that it would be better to use a checkbox (enabled) or radio buttons.

image
cp2004 commented 3 years ago

Would you mind doing a quick test to see if it does round it when sent to the printer? It's using a generic template, I guess we need more conditional work here. Problem is, in Marlin there's no consistency and I always end up with really specific cases, when I was trying to make generic definitions....

Anyway, I will see about fixing it for 3.1.0, I think there's probably something else like that around too.

KenLucke commented 3 years ago

Everything's printing right now, so I can't test, but a simple change to a checkbox (☑ signifying a "1" when stored) or Radio Button set ( ʘ Enabled O Disabled ), to me, would be far simpler than trying to specific-case conditionals.

KenLucke commented 3 years ago

OK, fired up the spare Pi and OP. Anything ≥ 1 when stored, displays as "1" when reloaded [although there's no telling what value is in the background). Anything < 1, when stored, displays as "0" - again, no telling whether it's floating point or integer in the background.

cp2004 commented 3 years ago

Thank you - that confirms that it's not really a critical bug that breaks everything when decimals 😄.

I agree, a checkbox would be easier on the user - but it makes it harder to send to the firmware, and further away from the command itself for those used to them. I will likely just lock it to 1/0 if possible, unless I can think of a smart way to do the checkbox.

KenLucke commented 3 years ago

I think that the average person who uses this kind of thing doesn't necessarily care about being "further away from the command itself" - they are just looking for functionality. Also, the average non-technical/coder user doesn't really inherently know that 1==enabled/0==disabled in most things.

I'm also having a hard time figuring out why it would be so hard to use a boolean (checkbox/radio button enabled) to represent an already boolean value (bed level enabled/disabled). I admit it's been a long time since I coded, but a simple if/then test should do it.

cp2004 commented 3 years ago

Yeah it is not difficult per se, it just requires extra steps in what is currently a very streamlined process, to convert something: true into S1 or something like that, and it requires this on all the ends where it comes in and out of the UI.

cp2004 commented 3 years ago

OK following a nearly two month break from this plugin... I've done this one.

The latest commits in this repository have switched the entire codebase over to a single source of truth, that makes maintainability easier with definitions for all the commands. This means I have been able to support Boolean values through this quite easily, so they are now checkboxes. So it's ready for 3.1.0 🙂

cp2004 commented 3 years ago

There's a release candidate available for testing this now: 3.1.0rc1. Feedback ticket #45 for any comments on the release. Stable release approx. 1 week depending on how the testing goes.

cp2004 commented 3 years ago

3.1.0 has been released