Sanitizing User Input when Calibrating: "ValueError: invalid literal for int() with base 10"

[rampageservices]

Hello,

I use a ssh program that works in powershell installed via www.scoop.sh. This does not happen in OSX terminal ssh.

I run into a ValueError whenever I press the backspace during calibration. While my situation is a edge case, the bigger issue is that It appears there is no sanitation done on user input for this step of the process.

The code in question is the following: int(input('Enter a PWM setting to test(0-1500)'))

My computer science professors used to call this sloppy ninja code. You are casting user input as an int without checking the value first. At the very least, the input should be checked to see if it is within bounds, 0-1500.

In the best case scenario, only digits should be assigned to the pwm variable.

See this stack overflow question for a few basic examples: https://stackoverflow.com/questions/17336943/removing-non-numeric-characters-from-a-string

Thanks!

using donkey v2.5.1 ...
Enter a PWM setting to test(0-1500)401
Enter a PWM setting to test(0-1500)402
Enter a PWM setting to test(0-1500)403
Enter a PWM setting to test(0-1500)4034^H^H4
Traceback (most recent call last):
  File "/usr/local/bin/donkey", line 11, in <module>
    sys.exit(execute_from_command_line())
  File "/usr/local/lib/python3.5/dist-packages/donkeycar/management/base.py", line 445, in execute_from_command_line
    c.run(args[2:])
  File "/usr/local/lib/python3.5/dist-packages/donkeycar/management/base.py", line 156, in run
    pmw = int(input('Enter a PWM setting to test(0-1500)'))
ValueError: invalid literal for int() with base 10: '4034\x08\x084'```

[bashbaugh]

Why don't you fix it yourself and create a pull request?

[bashbaugh]

Also, In my opinion, people should be able to read the prompt and enter a number between 0-1500. If they don't, I think python's error message is good enough to fix the problem.

[rampageservices]

Sure, I can invest the time to fix it and submit a pull request. However, the point of creating an issue is to discuss the issue before any action is taken. Maybe, someone has already written their own fix, maybe someone feels strongly that they want to submit the pull request.

Further, you are entitled to your opinion but I don't agree with your sentiment.

If Donkey car wants to see new user adoption the essential function of calibration needs to be bulletproof. Otherwise, users will get frustrated and no longer work on the project having never even gotten to the fun part of actually seeing the car drive itself.

Further, the error message is nonsense to any non programmer.

I am a strong believer in failing gracefully instead of failing hard. In this case, this is a hard failure that should be remediated.

[bashbaugh]

I see your point; I was not trying to say that your idea was bad. I was just trying to point out that, in my opinion, donkey developers have been focusing on other usability issues that such as getting donkey to work on rpib+, etc.; issues that could be much more non-programmer-deterring than this issue. But I also agree with you - the better and more graceful donkey's interface is; the more easy-to-use, glitch-free, and understandable donkey is, the more users we'll be able to accumulate.

As for the topic of creating a pull request, it looks like no one else is working on this issue right now so you could probably go ahead and fix this issue on your own if you wanted to - I can't see why anyone would have a problem with that. As for someone else doing it, I don't think anyone else will be getting around to fixing this issue any time soon (except for me, maybe).

the point of creating an issue is to discuss the issue before any action is taken.

I agree with you. And that's what were doing right now :smile:.

[bashbaugh]

325

[wallarug]

I agree with @rampageservices

There should really be better checks in place before sending the values to any hardware attached to the system.

Casting is bad practice in most languages and should be avoided. There are many strange issues that are caused by casting that are difficult to debug. The only time casting should be acceptable is when paired with some sanitising code to check the input by the user is valid.

I think something like the below should be included in the donkey car library so that it is less likely that someone is going to damage their car:

. . .
# checks if the given value to outside the acceptable range to avoid burn out of 300 - 460
# outputs a warning to the console.
def large_warn(x):
    if x < 300 or x > 460:
        print("[WARNING] You are using values outside the acceptable range.  Only continue if you \
                     know what you are doing and understand the risks.")
   else:
        continue

def sanitise(x, min, max):
    if x.isdecimal() and x.isdigit():
        try:
            val = int(x)
        except ValueError:
            return False
        else:
            if x < max and x > min:
                large_warn(x)
                return True
           else:
              return False
    else:
        return False

. . .
            pwm = input('Enter a PWM setting to test({}-{}): '.format(self.pwm_min, self.pwm_max))
            if sanitise(pwm, self.pwm_min, self.pwm_max):
                confirm = input("Confirm use of value {0}  (press any key) ".format(int(pwm))
                c.run(pwm)
            else:
                print("Only enter numbers in the range of 0 - 1500. Try Again.")

Something like above would be robust.

[rampageservices]

This issue was closed hastily. A better solution was proposed and was not discussed.