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

Password via G-code and MarlinUI #18399

Merged
merged 56 commits into from
Aug 9, 2020

Conversation

sherwin-dc
Copy link
Contributor

@sherwin-dc sherwin-dc commented Jun 24, 2020

Description

A settable passcode for the printer that can configured to ask for the passcode:

  • On startup
  • To access 'Print from Media' Menu
  • After a print from SD card stops or aborts

The number of digits for this passcode can be configured (1 to 9 digits). The passcode can be entered using a rotatory knob.

Note: It seems the touch UI includes a lockscreen, but this should bring the feature to more printers that do not have a touch screen.

Benefits

The majority of users might not need this. However, it would be useful if the printer is kept in a shared location such as:

  • At home, where the user may not want others (eg small children) to play with he printer's settings.
  • In a work environment (similar to how copier machines have passwords).

This would allow someone to enter their passcode when booting the printer, start a print, and have it automatically lock when the print ends.

Limitations

This is not meant to be a security feature, but a simple form of Access control.
The password can be circumvented by:

  • removing it from the Password Settings / EEPROM menu if the printer is unlocked
  • sending remote gcode commands, even if the printer is locked
  • removing the password through the menu while printing
    The password is not likely to stop someone who knows their way around a 3D printer.

Misc

I have tested this on my Ender 3 Pro but I am not sure what kind of LCDs this would work on. This feature makes use of the LCD Menu functions.

Password_screen

@sherwin-dc sherwin-dc force-pushed the bugfix-2.0.x branch 2 times, most recently from 3531571 to f045406 Compare June 25, 2020 07:36
@sherwin-dc

This comment has been minimized.

@sherwin-dc sherwin-dc force-pushed the bugfix-2.0.x branch 2 times, most recently from c29f2d1 to 1554fa9 Compare June 27, 2020 03:42
@sherwin-dc sherwin-dc marked this pull request as draft June 28, 2020 06:03
before merging with main repo

readded upstream config files

added switches in Configuration.h for password feature

Added Sanity Checks

Put back default environment as mega2560

default platformio.ini file

config files

remove extra config files

add newlines that were removed

minor edits
@sherwin-dc sherwin-dc marked this pull request as ready for review June 28, 2020 06:56
@sherwin-dc sherwin-dc changed the title Printer Passcode Add Printer Passcode for Non-touch Screens Jul 7, 2020
@thinkyhead
Copy link
Member

I've cleaned this up a bit and encapsulated the feature in a static class. The code is okay for what it does, but it needs some support at a deeper level to permit putting the machine into a "LOCKED" state where nothing will operate except the password menu and all external host control can also be blocked. To get better support for hosts, a G-code should be added for setting the password, locking the machine, and authenticating from a host.

@thinkyhead
Copy link
Member

I'll get into testing this soon and do some refinement of the input method and the data storage. A custom input screen for password digits (similar to the select_screen stuff) should not be too difficult to put together.

@sherwin-dc
Copy link
Contributor Author

ok, I dont think the problem is with pow(), as outputting pow(10,8) gives the correct value. However, I'm still not getting the value of the password keyed in and not sure what the problem is.

In the latest commit under Password::digit_entered() if I add a serial message for debugging:

const uint32_t multiplier = LROUND(pow(10, PASSWORD_LENGTH - 1 - digit_no));
SERIAL_ECHOLNPAIR("multiplier is ", multiplier);
value_entry += digit * multiplier;
string[digit_no++] = '0' + digit;

and try to set a 9 digit password one digit at a time using the LCD I get the following output:

multiplier is 99999968
multiplier is 9999984
multiplier is 1000000
multiplier is 100000
multiplier is 10000
multiplier is 1000
multiplier is 100
multiplier is 10
multiplier is 1

However simply outputting pow(10, PASSWORD_LENGTH - 1 - 0) gives the correct multiplier. I have tried changing the first line slightly such as using CEIL() on or casting to an int the second pow() argument but still get the same output as long as digit_no is included.

@AnHardt
Copy link
Contributor

AnHardt commented Jul 16, 2020

Often the print routines for printing floats are using tricks like: If the difference to a round decimal value is smaller than one LSB of the mantissa print the round value. For most of us it is disappointing if Serial.println(3.0 * 1.0/3.0); is not printing 1 . Especially in Arduino context that is very possible. Checked! It does print 1.00 . Try stupid things like Serial.println(pow(10,8)+1.0); resulting in 100000000.00.

  Serial.println(pow(2,24)-1); // 16777215.00
  Serial.println(pow(2,24));   // 16777216.00
  Serial.println(pow(2,24)+1); // 16777216.00

Fact stays - single precision numbers have no exact representation if the mantissa is above 2^23 = 8,388,608. With denormalization pow(2,24) is possible.
So the formerly used loop - staying in integer, had a chance to be correct, not so the pow() in single precision float.

https://en.wikipedia.org/wiki/Single-precision_floating-point_format

@sherwin-dc
Copy link
Contributor Author

I think in this case it may not be pow() thats the problem. It looks like it uses double precision floats.

As for the actual problem, I am not sure why it is happening, or if it can be reproduced on other machines.

In any case, using the for loop did avoid the problem on my machine. Furthermore, when I tried changing both the pow() functions to the loops it reduced the program size by about 700 bytes. Perhaps this is because my mainboard does not have an FPU and more instructions are required to do pow() with floats. So since floats arent needed it would be better to avoid them completely to at least save program memory.

@AnHardt
Copy link
Contributor

AnHardt commented Jul 18, 2020

It looks like it uses double precision floats.

It would be an error if the code would require to work in double. The AVRs do not have 'double' just and only 'single precision float'. The code would not work there on.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 18, 2020

True that floats aren't needed. And if we want to avoid pow there is always this trick:

#define FIFTH_POWER 5
constexpr long ten_thousand = CAT(10e, DECREMENT(FIFTH_POWER));

@sherwin-dc
Copy link
Contributor Author

I changed the pow() function to CAT(1e, PASSWORD_LENGTH). If using the LCD to key in the passcode it now divides the multiplier by 10 after each digit entered, so it should be better than the for loop.

I think the only thing left would be the dedicated password input screen, though the code should work fine as it is.

@thinkyhead thinkyhead merged commit 852e5ae into MarlinFirmware:bugfix-2.0.x Aug 9, 2020
@thinkyhead thinkyhead changed the title Add Printer Passcode for Non-touch Screens Password via G-code and MarlinUI Aug 9, 2020
Speaka pushed a commit to Speaka/Marlin that referenced this pull request Aug 13, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
@charakaSamawry
Copy link

just wondering, would this feature play nicely with the BTT TFTs? using the tft35 v3 with a BTT SKR Mini v2.
I have a feeling it would need to be implemented on the host firmware before i can enable it in marlin firmware, no?

@thisiskeithb
Copy link
Member

just wondering, would this feature play nicely with the BTT TFTs? using the tft35 v3 with a BTT SKR Mini v2.
I have a feeling it would need to be implemented on the host firmware before i can enable it in marlin firmware, no?

BigTreeTech's TFT firmware would need to be updated to support this feature.

@charakaSamawry
Copy link

As I thought, thank you.

@sherwin-dc
Copy link
Contributor Author

By the way I noticed the M512 gcode has been changed to use S instead of N to input the new password.

However the comments in the configuration.h file still reflect N being used:

  #define PASSWORD_CHANGE_GCODE             // Change the password with M512 P<old> N<new>.

@thisiskeithb
Copy link
Member

By the way I noticed the M512 gcode has been changed to use S instead of N to input the new password.

However the comments in the configuration.h file still reflect N being used

Thanks for the heads up! I've put in #19108 to address this.

albertogg pushed a commit to albertogg/Marlin that referenced this pull request Aug 31, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
vgadreau pushed a commit to vgadreau/Marlin that referenced this pull request Dec 9, 2020
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
kageurufu pushed a commit to CR30-Users/Marlin-CR30 that referenced this pull request Apr 30, 2021
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants