Fixed the usage of int.from_bytes and int.to_bytes #64
Conversation
… byteorder value Fixed DHCP and DNS using little-endian instead of big-endian for parsing bytes
|
Thanks for the fix. This does seem to match the CPython docs: https://docs.python.org/3/library/stdtypes.html#int.to_bytes CircuitPython docs don't show an example. They do mention being compatible with CPython, but it's not clear to me if it means the entire API or just the positionality of the arguments: https://docs.circuitpython.org/en/latest/docs/library/builtins.html?highlight=int.to_bytes#int.to_bytes I am also perplexed about how this could have been working in the past. Perhaps at some point the core had a different API than CPython but was adjusted to match somewhere along the way and this wasn't updated? Or perhaps this never did work, but isn't needed in all cases so no one noticed till now. I'll try to test this out on hardware tomorrow. |
|
My testing was performed on a Feather RP2040 with Ethernet Featherwing connected circuitpython versions I am getting this exception raised when I try to test the simpletest script with the branch from this PR: Adding some additional debugging prints it seems that Which then raises the above error. Stepping back through the code a bit further into Adafruit_CircuitPython_Wiznet5k/adafruit_wiznet5k/adafruit_wiznet5k_dhcp.py Lines 253 to 256 in a7d28e3 I'm getting the single "f" printed from this, but I don't know what it is supposed to indicate or in what situation it would be expected to arise. Honestly though I'm not sure if my results are due to any of the changes in this PR. I am getting basically the same errors when I try to run the current version of the driver library also. Perhaps my router is sending something "weird" that this device or current driver can't handle Here is the full output of the simpletest with |
|
The DHCP server is SUPPOSED to send a response with the same 4-byte XID field as the request from the DHCP client. The The return value in the non-error case from the function seems to be (int, bytes-of-length-4) but in the error cases it is (int, int). At first glance, I don't see why the changes in this PR would lead to this problem. did you test with the released version or with the tip of main before this PR? |
|
I see, thank you. I guess in my case the DHCP server could be returning something different for some reason. I added some additional prints and saw these values for xid on one run: Which are similar, but do not match. It seems the values do change from run to run as well. Aftrl ctrl-C + ctrl-D it's two different values but similarly the xid one has the first hex byte 1 lower than the initial xid.
I did test with the released version installed via circup and do get the same exceptions raised by that version. I do think it's likely unrelated to these changes. But perhaps an opportunity to change the return types to the ones expected by the code that receives these values back and/or ideally raise a more descriptive error about the xid mismatch if that is a fatal error that we can't continue after. |
|
fwiw, haven't tried PR yet, but I get the "f" return value intermittently on 5100. Not sure about 5500, there are none in my scrollback. But it's rare, and recovers. Also, not to muddy the waters, but note that WIZnet DHCP behavior can vary based on LAN config: |
There was a problem hiding this comment.
Testing on Adafruit CircuitPython 8.0.0-beta.0 on 2022-08-18; W5500-EVB-Pico with rp2040 confirms ValueError: Invalid byteorder without the PR code, and good operation with the PR code.
Testing on Adafruit CircuitPython 7.3.3 on 2022-08-29; Adafruit Feather RP2040 with rp2040 (and as old as 7.2.5) with Adafruit Ethernet FeatherWing (W5500) gives good results with the released v1.12.11 library, but ValueError: Invalid byteorder with 8.0.0-beta.0.
Similarly, Adafruit CircuitPython 7.3.3 on 2022-08-29; W5100S-EVB-Pico with rp2040 is fine, but Adafruit CircuitPython 8.0.0-beta.0 on 2022-08-18; W5100S-EVB-Pico with rp2040 gets ValueError: Invalid byteorder with the released v1.12.11 library.
It seems to be related to CP 8, though no issues or PRs stand out to me. CP 7.3.0 updated micropython to 1.18 and pico-sdk to 1.3.1, but there was nothing obvious to me in those and 7.3.3 runs fine.
Addendum:
The PR change appears to be backward-compatible also. The W5500-EVB-Pico runs fine with Adafruit CircuitPython 7.3.3 on 2022-08-29; W5100S-EVB-Pico with rp2040 and the PR-modified WIZnet library. Same backward-compatible results on the FeatherWing.
I'm inclined to approve the PR unless there are objections. We should still try to figure out where / why the apparent core change occurred.
Addendum: This seems relevant: adafruit/circuitpython@ac9cb93 (PR# adafruit/circuitpython#6562) ...If I read that right, in CP 7, unless the parameter is exactly "little", it will be treated as "big", so "b", "l", etc. get treated as big, and that's why it worked before. Testing a few conversions in the 7.x REPL bears this out. This commit was tagged 8.0.0-beta.0.
Updating https://github.com/adafruit/Adafruit_CircuitPython_MCP9600 to 1.3.6 from 1.3.5: > Merge pull request adafruit/Adafruit_CircuitPython_MCP9600#21 from kattni/doc-and-comment-fix Updating https://github.com/adafruit/Adafruit_CircuitPython_TCA8418 to 1.0.3 from 1.0.2: > Merge pull request adafruit/Adafruit_CircuitPython_TCA8418#8 from adafruit/3x4_oled > Merge pull request adafruit/Adafruit_CircuitPython_TCA8418#7 from adafruit/3x4_oled Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 1.12.12 from 1.12.11: > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#64 from alexsporn/fix/endiannes Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE_Beacon to 1.0.1 from 1.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_BLE_Beacon#1 from tekktrik/main Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 5.5.1 from 5.5.0: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#123 from rtwfroody/eagain2 Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Added the following libraries: Adafruit_CircuitPython_BLE_Beacon
int.from_bytesandint.to_bytesto pass the proper byteorder value. The correct values are"big"and"little"not"b"and"l""l"instead of"big"Not sure how this was working before. Maybe older versions ignored the wrong parameters and used big-endian by default. Without this fix I could not get it to work on my board, I was always getting a
ValueError: Invalid byteorderwhen doing DHCP.This was tested with: