Prevent I2C lockups #32
Conversation
void clearRecoveryCount(); uint16_t getRecoveryCount();
void clearRecoveryCount(); uint16_t getRecoveryCount();
…o prevent lockup when the I2C bus fails to respond as expected. In addition, a lockup_recovery_count variable was added, along with lockup_counter 'get' and 'clear' functions
|
Thanks for your contribution! Having some fix for the lockups would be great, however, one problem with timeouts is that some applications might have significant clock stretching, would could be broken by a change like this. As for the PR itself, your commits contains ton of whitespace changes, which makes the PR hard to review and impossible to merge. Commit messages do not need to contain the date, git tracks this. I'm not sure what |
|
Matthijs,
Thanks for the prompt and detailed reply. As you may have guessed, I'm
pretty new at this, so please bear with me ;-).
I've no idea what you mean about the white space - I just added the code
changes and committed/pushed to my repo. Do you mean extra lines, or maybe
whitespace added due to changes between tabs and spaces (I think Win10
likes to change things between tabs and spaces). In any case, do you have
a recommendation for addressing this issue?
Sorry about the dates - how do I fix that?
'gfp' is my initials; I grew used to doing this in the old days (there was
software before Git) so other collaborators know who the culprit was
Not sure what you mean by ' function declarations are best added in the
same commit as the related function definitions, since these are part of a
single logical change.' Was it because I used separate commits for each
file?
TIA,
Frank
…On Thu, Aug 16, 2018 at 3:56 PM, Matthijs Kooijman ***@***.*** > wrote:
Thanks for your contribution! Having some fix for the lockups would be
great, however, one problem with timeouts is that some applications might
have significant clock stretching, would could be broken by a change like
this.
As for the PR itself, your commits contains ton of whitespace changes,
which makes the PR hard to review and impossible to merge. Commit messages
do not need to contain the date, git tracks this. I'm not sure what gfp
means? Finally, function declarations are best added in the same commit as
the related function definitions, since these are part of a single logical
change.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#32 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD0967J2_nx90aO1LtzJr6qC2jjTrutxks5uRc5vgaJpZM4WAdrp>
.
--
G.Frank Paynter, PhD
OSU ESL Research Scientist (ret)
EM Workbench LLC
614 594-2078 (home office)
|
If you look at the diff of the commit (here by clicking "commits" or "Files changed") or in whatever program you're using to commit, which is always a good idea before creating a PR), you'll see that a lot of lines are changed, but most of them only change whitespace or indentation. These changes are meaningless and mess up history, so we'd rather not have them. As for removing them - that might not be so easy, unfortunately. You might be able to do some search-replacing to undo the changes (and use rebase / history rewriting to actually fix the existing commits), otherwise it might be easier to start from scratch and make sure your editor only changes what you're typing, and does not try to "fix" everything or force a particular indentation style (best practice is to preserve the existing style).
You'll have to use some history-rewriting in git, such as the rebase command. That can be a bit complicated, so I can't give you the 101 here. Google should have plenty info, though. IIRC https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History is a good resource (also the rest of that book).
I suspected as much: no need, git will remember you made the change. This also applies for adding comments in files stating the date and the change made. There are such comments in existing files, but they are no longer used.
Yup, it is better to use a single commit for each logical change, which makes them more self-contained and easy to review (now, and in years to come). That changes are in different files is completely irrelevant - git can also tell us that already :-) |
|
One more thing: If you manage to get things fixed up with history-rewriting, you can do a force push to your github branch, and this PR will be automatically updated, replacing the old commits with your new changes. |
|
Matthijs,
I have given up for now - it is clear I don't know what I'm doing, and I
don't want to have to become a Git expert just so I can meet your standards
for a pull request.
I have deleted the fork I made, and deleted my local repository. Maybe
when I have some more time I'll try starting over from scratch.
Frank
…On Thu, Aug 16, 2018 at 4:32 PM, Matthijs Kooijman ***@***.*** > wrote:
One more thing: If you manage to get things fixed up with
history-rewriting, you can do a *force push* to your github branch, and
this PR will be automatically updated, replacing the old commits with your
new changes.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#32 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD096_7udzzpPG62g9jSIkRpbp5gz8d8ks5uRdbPgaJpZM4WAdrp>
.
--
G.Frank Paynter, PhD
OSU ESL Research Scientist (ret)
EM Workbench LLC
614 594-2078 (home office)
|
|
|
|
You are kidding me - you are getting around to this almost three years
after the fact!?
This whole thing has been OBE'd
…On Fri, Apr 9, 2021 at 12:14 PM CLAassistant ***@***.***> wrote:
[image: CLA assistant check]
<https://cla-assistant.io/arduino/ArduinoCore-avr?pullRequest=32>
Thank you for your submission! We really appreciate it. Like many open
source projects, we ask that you sign our Contributor License Agreement
<https://cla-assistant.io/arduino/ArduinoCore-avr?pullRequest=32> before
we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us
recheck
<https://cla-assistant.io/check/arduino/ArduinoCore-avr?pullRequest=32>
it.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#32 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA6T3257YOO7PFDGPIUW3DTTH4RW3ANCNFSM4FQB3LUQ>
.
--
G.Frank Paynter, PhD
OSU ESL Research Scientist (ret)
EM Workbench LLC
614 638-6749 (cell)
|
|
Hm, it seems this one was left open, but a similar change has since been merged with #107, so I'll go ahead and close this PR. The CLA assistent comment was just an automated comment that was posted to all open PRs, so can be ignored here :-) |
back in 2010 Nirea (see http://forum.arduino.cc/index.php/topic,19624.0.html) posted an effective method for preventing I2C lockups, but somehow his mods (or any of several different attempts at this) never made it into the baseline code. This pull request proposes his changes, plus the addition of a 'lockup_reset_counter' and associated 'get' and 'clear' functions.
The changes involve very straightforward modifications to Wire.cpp/h and twi.c/h. I have tested these mods on an Arduino Mega 2560 with 2 slave devices (an Adafruit RTC and a DFRobots IMU6050 module); before the mods, the best I could do was a few hours without a lockup, but after the mods my test project ran for over three days (60+ hours) before I stopped the test.
TIA,
Frank