-
Notifications
You must be signed in to change notification settings - Fork 221
Comments
Check for ValueError exception when reading PDOMapping entrys from ED...#386
Check for ValueError exception when reading PDOMapping entrys from ED... #386Obsnold wants to merge 2 commits intocanopen-python:master from
Conversation
@acolomb
acolomb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this possibly hides real errors? Like if someone writes PDOMapping=yes
Wouldn't it be better to check explicitly for an empty string and convert it to zero?
Obsnold
commented
Jul 12, 2023
Sure that sounds good to me, I'll try and update ti today.
...y string, this way we don't mask any other errors
Obsnold
commented
Jul 12, 2023
Alright I've updated it, please take another look.
Thanks!
Obsnold
commented
Aug 2, 2023
@acolomb have you had time to look over the changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it is still possible to have invalid input.
Maybe something like this?
var.pdo_mappable = eds.get(section, "PDOMapping", fallback="0") not in ("", "0")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want the invalid input to throw an exception so real errors are caught, though I guess it's a question whether PDOMapping= is valid input or not?
I no longer have access to the EDS files that were causing the issue so I don't have anything to test against.
If I have time I'll try and poke around to see if I can find any other files that exhibit this issue.
it does make me wonder if the handling of invalid input on the other entries should also throw an error?
...S files.
Resolves the following issue: