search

arduino-libraries / Ethernet

Ethernet Library for Arduino
http://arduino.cc/
259 stars 264 forks source link

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

Open masterx1981 opened 3 years ago

masterx1981 commented 3 years ago
CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

JAndrassy commented 2 years ago

@masterx1981 your additional commits go into the PR

masterx1981 commented 2 years ago

@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 commented 2 years ago

Hello guys

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

Thanks Felix

masterx1981 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.

masterx1981 commented 2 years ago

@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?

per1234 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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!!