OSError when setting too large speed_sp

[maweki]

• ev3dev version: 4.4.68-20-ev3dev-ev3

One of my students ran into an issue where they set motor.run_forever for a large motor via a formula. Once the result reached 1050 they got a OSError: InvalidArgument. 1050 is the max_speed. I think the library should cap the input at 1050 (or the max_speed of the corresponding tacho-motor) or throw a ValueError instead.
The error message, in any way, is very cryptic considering how easy it is to trigger.

speed_R was set to something larger than the max_speed.

Traceback (most recent call last):
  File "./braitenberg4b.py", line 26, in <module>
    motor_R.run_forever(speed_sp = speed_R)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 793, in run_forever
    setattr(self, key, kwargs[key])
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 583, in speed_sp
    self._speed_sp = self.set_attr_int(self._speed_sp, 'speed_sp', value)
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 217, in set_attr_int
    return self._set_attribute(attribute, name, str(int(value)))
  File "/usr/lib/python3/dist-packages/ev3dev/core.py", line 206, in _set_attribute
    attribute.write(value.encode())
OSError: [Errno 22] Invalid argument

[ddemidov]

An alternative would be to incorporate the limit into the formula that was used for controlling the motor. The error you got is returned from the driver itself and should be the same across all bindings. We could probably cap the speed in the library code, but I don't like the idea of always doing something that is only necessary sometimes. @WasabiFan, @dwalton76, what do you think?

[maweki]

@ddemidov I understand that capping may not be intended (and inconsistent) but then I'd really expect for the Python bindings to at least raise a ValueError.

My student asked me to look at their code and they thought they did something wrong with rounding and the types because the message was so generic.

[WasabiFan]

I think it would be reasonable to explicitly catch this error and re-throw aValueError instead. We should be able to check for EINVAL (22) in a try block specifically around the line which sets the motor apeed and raise the original exception again if it isn't EINVAL.

[ddemidov]

We should be able to check for EINVAL (22) in a try block specifically around the line which sets the motor apeed and raise the original exception again if it isn't EINVAL

That block is autogenerated. We could add a try/catch block around the generic setter here though.

[WasabiFan]

I was thinking we could add it to the run_* commands instead; in the case of direct property setters, it's fairly clear what is "invalid" about the single parameter passed in. It may make it a bit inconsistent, but I think it would work reasonably well overall.

[ddemidov]

we could add it to the run_* commands instead

Yes, that could work. Do we catch exceptions just for speed_sp or for anything else as well?

[WasabiFan]

Ugh, after taking a look at our run_* implementations, this isn't as easy as I was hoping.

I think our goal would be to provide an error that specifically calls out "speed"; in that regard, we need to use individual try blocks which look at the particular property which is being set. I guess we would need to wrap just the setattr call and check the current loop value... Thoughts?

[ddemidov]

I think that is doable but is going to be on the ugly side of things. I wont be able to implement this myself for some time (I am on vacation now), so pull requests are welcome :).

[ddemidov]

May be catching exceptions in the generic Device._set_attribute followed by a call to generic exception analyzer would be less ugly and work outside of run_* functions as well.

[WasabiFan]

Hmmm... I'll take a look at this later today or tomorrow and see what I can come up with. Of course, with the way our time zones line up, my "later today" is your "early morning" 😆

[WasabiFan]

Here's my current thinking: I'll wrap the important bits of _set_attribute in a try block. I'll define a _raise_friendly_access_error(driver_error, attribute) method which I call from the except block. That new method checks for combinations of error codes and properties and prints the appropriate message; something like this:

if not isinstance(driver_error, OSError):
    raise driver_error

if driver_error.errno == errno.EINVAL:
    if attribute == "speed_sp":
        raise Exception("The given speed value was out of range")
    raise Exception("One or more arguments were out of range or invalid")
# etc.

(apologies if I got the Python specifics mixed up, I switch between a lot of languages 😆)

Does that seem reasonable? The next question I have is this: if we see that the error was EINVAL for speed_sp, for example, do we want to attempt to access the max_speed file/attribute to be able to include that in the message? Or is that over-complicating things?

[ddemidov]

Here's my current thinking

I think this should work. The exception handler also has potential for extending the functionality (in case we need to process exceptions from other attributes).

do we want to attempt to access the max_speed file/attribute to be able to include that in the message?

I think that would be helpful.

[WasabiFan]

Whoops, looks like I forgot to post here!
I implemented this error handling but haven't gotten a chance to test it yet; I just pushed a fix for syntax to my branch. I might try this out later today while working on other fixes.

The implementation is here: https://github.com/rhempel/ev3dev-lang-python/tree/throw-friendly-errors

[maweki]

I also haven't tested it but two things: we really should raise a ValueError. The original issue was that the error was too general. The solution can't be to just throw an even more generic Exception.

And the exception also occurs if one sets the speed lower than negative max speed but in your code you only cap it on the positive side.

[WasabiFan]

I also haven't tested it but two things: we really should raise a ValueError. The original issue was that the error was too general. The solution can't be to just throw an even more generic Exception.

The goal is to make the exceptions as specific as possible -- it isn't an even more generic Exception, it's a detailed Exception which even includes the allowed max value (this varies depending on the motor).

And the exception also occurs if one sets the speed lower than negative max speed but in your code you only cap it on the positive side.

Sorry, I'm not sure what you mean. Where do we only consider the positive side? That sounds like it might be a bug, but I can't find such a location.