-
Notifications
You must be signed in to change notification settings - Fork 35
Add W5100S chip #48
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
Add W5100S chip #48
Conversation
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.
This is a great pull request, I have a few minor questions/changes.
The PR is also failing at https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k/runs/4235403094?check_suite_focus=true
We have a guide about checking your pull request's code on https://learn.adafruit.com/improve-your-code-with-pylint/check-your-code-with-pre-commit, please follow this guide and ask me any questions if you need help.
@min-hs The PR is still failing at https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k/runs/4246378618?check_suite_focus=true We have a guide about checking your pull request's code on https://learn.adafruit.com/improve-your-code-with-pylint/check-your-code-with-pre-commit, please follow this guide and ask me any questions if you need help. I'll merge in once it passes, thanks for making the requested changes. |
We're trying to use W5100 additionally with W5100S, so if possible, we'll promote it together. |
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.
Tested the PR code on a W5100S-EVB-Pico:
Adafruit CircuitPython 7.1.0-beta.0 on 2021-11-12; Raspberry Pi Pico with rp2040
code.py output:
Wiznet5k Ping Test (no DHCP)
Chip Version: w5100s
MAC Address: ['0x0', '0x1', '0x2', '0x3', '0x4', '0x5']
My IP address is: 192.168.1.100
Nice!
@min-hs The PR is still failing at https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k/runs/4246378618?check_suite_focus=true We have a guide about checking your pull request's code on https://learn.adafruit.com/improve-your-code-with-pylint/check-your-code-with-pre-commit, please follow this guide and ask me any questions if you need help. I'll merge in once it passes Actions. |
I was able to make the changes for black and pylint. I have everything passing pre-commit checks except reuse - I'm unfamiliar with it, so I'm not sure what the traceback is trying to tell me.
|
@AdamCummick reuse is used for enforcing licensing on all of the published code and files (files will fail the reuse check if they do not reference a valid license. However the specific issue that you posted the stacktrace for is actually a bit of a red herring I think. The root cause of that issue is python 3.6. I'm not up on the specifics but something changed at some point within the debian builds of python that made them start raising that error for us sometimes. The fix generally has been to update the actions files to use a CI container with python 3.7 at minimum. If you have the option to try running the checks with python 3.7 or greater give that a shot I think it should get you past that error. |
Thanks @FoamyGuy , all good now. Since this isn't my fork, how should I submit the changes? |
@AdamCummick You may try opening a new PR |
@AdamCummick yep new PR is probably best. You can start your branch from min-hs/main branch to preserve the commits from this PR. |
Going with #49, closing.. |
In the existing Adafurit_wiznet5k library, only W5500 was recognized.
I revised the library to make W5100S possible. Related examples can also be found in the link below.