Skip to content
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

Fix issue #140, #105 and #151 #152

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

masterx1981
Copy link

@masterx1981 masterx1981 commented Dec 26, 2020

Addedd last few missing registers for W5500
Merged the ethernet library with the ICMPPing library
Addeed automatic spi setting for industruino
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

felmue and others added 5 commits June 4, 2021 05:42
From the W5500 IPRAW documentation (http://www.wizwiki.net/wiki/lib/exe/fetch.php/products:w5500:w5500_ap_ipraw_v100e.pdf):
'After initialization of W5500, the Ping Reply is processed automatically. However, be aware that the Hardwired Ping Reply Logic is disabled if ICMP is opened as SOCKET n in IPRAW mode.'
Merged some ESP8266 code from the main ERthernet lib
Merged the ifndef htons section from main branch
@JAndrassy
Copy link
Contributor

@masterx1981 your additional commits go into the PR

@masterx1981
Copy link
Author

@masterx1981 your additional commits go into the PR

Sorry i not know Github very well, the CLA warns me that @felmue not have signed tge agreement so the PR seem locked

@felmue
Copy link

felmue commented Jan 1, 2022

Hello guys

I didn't realize you're waiting on me signing the CLA. Sorry about that.

Thanks
Felix

@masterx1981
Copy link
Author

Hello guys

I didn't realize you're waiting on me signing the CLA. Sorry about that.

Thanks Felix

No problem, i've noticed it yesterday... i've merged some things of the main branch to our library (minor changes), if they want to merge our version to the main one, i think that it's better.

@JAndrassy
Copy link
Contributor

JAndrassy commented Jan 1, 2022

maybe check again what you have in this PR. files changed only with commented out debugging.
https://github.com/arduino-libraries/Ethernet/pull/152/files

@masterx1981
Copy link
Author

maybe check again what you have in this PR. files changed only with commented out debugging. https://github.com/arduino-libraries/Ethernet/pull/152/files

Sorry i not understand. I see all the differences from my fork to the basic fork. So, some debug commented out, included some files for the icmp support, corrected registry definitions for w5500, and minor other things.

@per1234
Copy link
Contributor

per1234 commented Jan 1, 2022

@masterx1981 every commit you push to the master branch of your fork is also added to this pull request, and would be introduced into the official Arduino Ethernet library if this PR was merged.

For this reason, you must dedicate the master branch exclusively to changes you wish to propose in this pull request, and not push any other changes to that branch. You can create separate branches to use to prepare additional distinct proposals, personal development, experimentation, etc.

Please remove any unrelated changes from this pull request. You might find Git's interactive rebase feature to be useful for this purpose. I'll share a overview of that approach:

  1. If you haven't already, clone your fork of this repository to your computer.
  2. Checkout the branch the PR was submitted from.
  3. Do an interactive rebase to adjust the commits as needed.
  4. Force push the changes back to your fork on GitHub.

Please let me know if you have any questions.

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jan 1, 2022
@masterx1981
Copy link
Author

@masterx1981 every commit you push to the master branch of your fork is also added to this pull request, and would be introduced into the official Arduino Ethernet library if this PR was merged.

For this reason, you must dedicate the master branch exclusively to changes you wish to propose in this pull request, and not push any other changes to that branch. You can create separate branches to use to prepare additional distinct proposals, personal development, experimentation, etc.

Please remove any unrelated changes from this pull request. You might find Git's interactive rebase feature to be useful for this purpose. I'll share a overview of that approach:

  1. If you haven't already, clone your fork of this repository to your computer.
  2. Checkout the branch the PR was submitted from.
  3. Do an interactive rebase to adjust the commits as needed.
  4. Force push the changes back to your fork on GitHub.

Please let me know if you have any questions.

Ooohhh, thanks for the explanation!
I've cleaned a bit the code, but as i've addressed other problems in the code other than the issue #105, it's better to create e brench for every mod and ask for a pull request for every of them?

@masterx1981 masterx1981 changed the title Update w5100.h Fix issue #140, #105 and #151 Jan 1, 2022
@per1234
Copy link
Contributor

per1234 commented Jan 3, 2022

it's better to create e brench for every mod and ask for a pull request for every of them?

Yes please.

Atomic pull requests which propose only one distinct change set are best practices. This makes it easier for interested parties to review. It also avoids the possibility of a "poison pill" situation where acceptance of a straightforward, non-controversial proposal is blocked due to it being bundled unnecessarily with a separate proposal that is difficult to evaluate, or not considered beneficial by all interested parties.

@masterx1981
Copy link
Author

masterx1981 commented Jan 5, 2022

it's better to create e brench for every mod and ask for a pull request for every of them?

Yes please.

Atomic pull requests which propose only one distinct change set are best practices. This makes it easier for interested parties to review. It also avoids the possibility of a "poison pill" situation where acceptance of a straightforward, non-controversial proposal is blocked due to it being bundled unnecessarily with a separate proposal that is difficult to evaluate, or not considered beneficial by all interested parties.

And, isn't better leave the master branch with all the mods, and create pull request on the branches with every single mod?
My bad, this pull request is on the master branch...
I need to study a bit how github works, i have little spare time and i always try to not read the documentation, sorry...

@per1234
Copy link
Contributor

per1234 commented Jan 5, 2022

And, isn't better leave the master branch with all the mods, and create pull request on the branches with every single mod?
My bad, this pull request is on the master branch...

I wouldn't worry about this pull request being from the master branch of your fork.

It is helpful to use a descriptively named branch for each pull request so that you will get a hint at the purpose of each branch from the name while you are navigating your repository. But as far as Arduino is concerned, it makes no difference what the branch you submitted the pull request from is named because the pull request itself should describe the proposal well enough.

@masterx1981
Copy link
Author

And, isn't better leave the master branch with all the mods, and create pull request on the branches with every single mod?
My bad, this pull request is on the master branch...

I wouldn't worry about this pull request being from the master branch of your fork.

It is helpful to use a descriptively named branch for each pull request so that you will get a hint at the purpose of each branch from the name while you are navigating your repository. But as far as Arduino is concerned, it makes no difference what the branch you submitted the pull request from is named because the pull request itself should describe the proposal well enough.

Wouldn't be better for who want to use my library if the master branch is the one with all the mods applied? As i see, the main ethernet library have a lot of uncommited requests, and can be a long wait for have them merged.

@per1234
Copy link
Contributor

per1234 commented Jan 15, 2022

Wouldn't be better for who want to use my library if the master branch is the one with all the mods applied?

It is up to you how you want to maintain your fork. Some would mirror the Arduino branches and create a dedicated branch for distribution of modified code to users (perhaps even making that one the default branch). Others would use master for that purpose.

As I mentioned above, the subject of which branch of a fork a PR is submitted from is moot from the perspective of the maintenance of Arduino's own library in this repository.

@budulinek
Copy link

Hi everyone,
would it be possible to merge this PR? I am looking for a fix to setRetransmissionTime() not working on W5500.
Thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants