Skip to content

Fixed MAC reverse byte order issue within the ESP32SPI library as it … #66

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

Merged
merged 5 commits into from
Feb 28, 2020
Merged

Fixed MAC reverse byte order issue within the ESP32SPI library as it … #66

merged 5 commits into from
Feb 28, 2020

Conversation

mytechnotalent
Copy link
Contributor

…was originally echoing the MAC address in reverse byte order. Added portable MAC function call within WSGISERVER.PY example to properly echo MAC address when running example.

…was originally echoing the MAC address in reverse byte order. Added portable MAC function call within WSGISERVER.PY example to properly echo MAC address when running example.
@mytechnotalent
Copy link
Contributor Author

@ladyada ganbatte done it :)

@ladyada
Copy link
Member

ladyada commented Aug 21, 2019

@brentru wanna try this and use an external tool to verify the MAC of the esp32

@anecdata
Copy link
Member

anecdata commented Aug 22, 2019

Minor point, but can we mark this as a breaking change in the release notes? Some people (like me) may have code that uses the old order.

@ladyada
Copy link
Member

ladyada commented Aug 22, 2019

why would it be a breaking change? usually you display the MAC, but code doesnt stop working because of the new order

@anecdata
Copy link
Member

I suppose it's not breaking within CP (and perhaps dumb on my part), but for example, I use an ID based on old order to ID the ESP unit to my server.

@ladyada
Copy link
Member

ladyada commented Aug 22, 2019

whatever y'all want to do. they're just numbers

@mytechnotalent
Copy link
Contributor Author

I don't want to break anyone's code. What I will do is add a new function to the adafruit_esp32spi.py library called MAC_address_actual to which you as the community can have a review on.

…ibrary calling the actual MAC address and providing a usage of the function in the webserver code within WSGISERVER.
…ibrary calling the actual MAC address and providing a usage of the function in the webserver code within WSGISERVER. This time with line breaks at end to not break Travis.
@mscosti
Copy link
Contributor

mscosti commented Aug 23, 2019

For what its worth, it appears that the nina-fw is purposely sending the order this way, and the arduino WiFiNina library doesn't do anything to adjust it. In the example usage you can see they're reversing the output of the call when printing it, but the byte array the .macAddress() function returns is left untouched.

I'm not sure why this is the way it is. Does it make sense to simply document this behavior, and perhaps implement a helper method that reverses it for use when printing?

@ladyada
Copy link
Member

ladyada commented Aug 25, 2019

i think reversing it in the example is also fine.

@kattni
Copy link
Contributor

kattni commented Jan 8, 2020

@brentru Can you test this if you did not already? It looks like it's otherwise ready to merge. Thanks!

@brentru
Copy link
Member

brentru commented Jan 8, 2020

@kattni I'll test later today. Assigning myself....

@brentru brentru self-requested a review January 8, 2020 18:46
Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The esp32spi.py has not been updated since a349bac. I am not sure why GitHub is not throwing a merge conflict but newer methods like get_time do not exist within this file.

@kattni
Copy link
Contributor

kattni commented Jan 29, 2020

@brentru I believe it's possible that since the changes don't conflict with that part of the file, merging this PR will not have any problems. If you're satisfied with the changes, I say we merge it and verify that the newer functionality is still in the file. We can always revert it if necessary.

@brentru
Copy link
Member

brentru commented Jan 29, 2020

@kattni Sounds good, I have no problems with the changes and would be OK to merge.

@kattni
Copy link
Contributor

kattni commented Jan 29, 2020

@brentru Please merge it and verify the functions in question are still present. I will release the library if you give the go ahead.

@FoamyGuy FoamyGuy merged commit fbbe977 into adafruit:master Feb 28, 2020
@FoamyGuy
Copy link
Contributor

I verified there were no merging conflicts. Also tested the changes afterward. The new property and edited example are working as expected. As a sanity check I also tested the updated version with an example unrelated to this change and everything checked out fine there as well.

adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 28, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_ATECC to 1.0.5 from 1.0.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_ATECC#9 from fgallaire/master

Updating https://github.com/adafruit/Adafruit_CircuitPython_BluefruitSPI to 1.0.5 from 1.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_BluefruitSPI#14 from jerryneedell/jerryn_unpack

Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 3.2.0 from 3.1.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#66 from mytechnotalent/ESP32-MAC-order-library-fix

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyBadger to 2.1.2 from 2.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#26 from kattni/qr-code-update
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#25 from kattni/rename-show
  > Merge pull request adafruit/Adafruit_CircuitPython_PyBadger#24 from kattni/pronoun-badge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants