Closed rshartog closed 8 years ago
Hi Eero,
I have never done a pull-request before, so hopefully this is correct. I have done fairly extensive testing of the change on my side -- including a final check of wiping my local stuff and testing straight from the branched code.
If you run the TinyWire_Stress_Master (Uno or other standard Arduino) and TinyWire_Stress_Slave (Tiny85) programs with the current usiTwiSlave.c file, you will see one successful transaction, data mismatches on several subsequent transactions, and then the usiTwiSlave.c will hang on line 406, "while ( tmphead == txTail );". This behavior is consistent with the usiTwiSlave.c issue addressed in this change.
If you run the same master/slave programs with usiTwiSlave.c file I submitted (1 lined moved), it will run successfully all day. I have typed some descriptions of the problem in the master/slave programs themselves.
I also have another, less fundament proposal to allow the full size of the Tx and Rx buffers to be used. In the current implementation in usiTwiSlave.c, the Tx and Rx buffers cannot fully wrap where the head equals the tail, so the available buffer sizes are really TWI_RX_BUFFER_SIZE - 1 and TWI_TX_BUFFER_SIZE - 1. This matters because the master program thinks it can send a specific number of bytes (TWI_RX_BUFFER_SIZE) in a transmit begin()/end() set and this number is really one less than the code would make it seem. The same is true for the requestEvent callback on the slave putting data into the Tx buffer.
Let me know if you're open to this change and I'll do another pull-request. The change would affect more lines (probably about 12) in usiTwiSlave.c, but the change will be obvious and it will not be nearly as subtle as the "single-byte send" issue in usiTwiSlave.c.
Thanks for all your work on TinyWireS, --Scott
Merged earlier PR and now we have conflicts. Try
git remote add upstream https://github.com/rambo/TinyWire.git # this is only needed once
git checkout master ; git fetch upstream ; git rebase upstream/master master ; git push
The latter line will update your local clones master branch from the upstream, then you can do git rebase master
in your rsh branch. While at it do git rebase -i HEAD~7
(or just a commit id of the "update usitwislave") and squash those add/delete/update "churn" commits into one.
Hi Eero,
In my opinion, the previous pull-request (#17) was an incorrect attempt at the same objective as my pull-request. This may sound arrogant, but I’ve looked at both pull-requests, and I recommend that you revert pull-request #17 and incorporate #18. At least you should examine both pull-requests and pick the one that makes the most sense to you.
I’m not going to pursue integrating #18 into the baseline with #17, because I’m not personally going to move to the version that has pull-request #17 incorporated.
I am curious to know if pull-request #17 passes the master/salve stress test programs that I submitted, but I’m not willing dismantle my current project to test it.
—Scott
On Feb 6, 2016, at 1:38 PM, Eero af Heurlin notifications@github.com wrote:
Merged earlier PR and now we have conflicts. Try
git remote add upstream https://github.com/rambo/TinyWire.git # this is only needed once git checkout master ; git fetch upstream ; git rebase upstream/master master ; git push The latter line will update your local clones master branch from the upstream, then you can do git rebase master in your rsh branch. While at it do git rebase -i HEAD~7 (or just a commit id of the "update usitwislave") and squash those add/delete/update "churn" commits into one.
— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/pull/18#issuecomment-180833939.
If you have not already read my prior email, please do.
I couldn’t figure out how to squash commits thru the web interface, so I have installed the GIT command line tools.
I have made two attempts, but it’s clear that I don’t know how to correctly pull / rebase / squash / push. I’m trying to do it within my branch without rebasing to the head of the master branch. That seems to work fine. I get a list of only my commits. The squash also seems to work in my local copy. However, when I push, I still see on the website all my original commits plus a new, empty commit for this push.
I’m very new to GitHub so any help would be appreciated.
I filtered the gunk out of my command history, and I think these are the actions that actually did anything:
git clone https://github.com/rshartog/TinyWire.git -b rsh git rebase -i HEAD~7 # edit file to squash git pull git push git rebase -i 550443b # rebase to branch point, edit to squash
vi TinyWireS/examples/TinyWireS_Stress_Master/TinyWireS_Stress_Master.ino git add TinyWireS/examples/TinyWireS_Stress_Master/TinyWireS_Stress_Master.ino git rebase --continue
vi TinyWireS/examples/TinyWireS_Stress_Slave/TinyWireS_Stress_Slave.ino git add TinyWireS/examples/TinyWireS_Stress_Slave/TinyWireS_Stress_Slave.ino git rebase --continue git push
Thanks, —Scott
On Feb 6, 2016, at 4:08 PM, Scott Hartog scott@hartog.net wrote:
Hi Eero,
In my opinion, the previous pull-request (#17) was an incorrect attempt at the same objective as my pull-request. This may sound arrogant, but I’ve looked at both pull-requests, and I recommend that you revert pull-request #17 and incorporate #18. At least you should examine both pull-requests and pick the one that makes the most sense to you.
I’m not going to pursue integrating #18 into the baseline with #17, because I’m not personally going to move to the version that has pull-request #17 incorporated.
I am curious to know if pull-request #17 passes the master/salve stress test programs that I submitted, but I’m not willing dismantle my current project to test it.
—Scott
On Feb 6, 2016, at 1:38 PM, Eero af Heurlin <notifications@github.com mailto:notifications@github.com> wrote:
Merged earlier PR and now we have conflicts. Try
git remote add upstream https://github.com/rambo/TinyWire.git https://github.com/rambo/TinyWire.git # this is only needed once git checkout master ; git fetch upstream ; git rebase upstream/master master ; git push The latter line will update your local clones master branch from the upstream, then you can do git rebase master in your rsh branch. While at it do git rebase -i HEAD~7 (or just a commit id of the "update usitwislave") and squash those add/delete/update "churn" commits into one.
— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/pull/18#issuecomment-180833939.
Yeah I reverted the previous PR, I did some rebasing and https://github.com/rambo/TinyWire/tree/fix_rsh should look good now, you can rename your local branch git branch -m rsh rsh_old
then fetch my branch git remote add upstream https://github.com/rambo/TinyWire.git && git fetch upstream && git checkout fix_rsh
then force push it on top of your old branch git push -f --set-upstream origin rsh
I think that should work... another option is that I close this PR and make a new one from that fixed branch and merge that instead :)
Hi Eero,
You're the GitHub expert. Let’s do whatever’s easier. It you need me to do something, please let me know (detailed instructions are good :) ).
I’m not sure that I didn’t mess up my branch in my attempts to squash the commits. The new commits appear to be empty, but it would be prudent for you to look it over. I have not intentionally changed anything in the last 6 or 7 hours.
—Scott
On Feb 6, 2016, at 5:40 PM, Eero af Heurlin notifications@github.com wrote:
Yeah I reverted the previous PR, I did some rebasing and https://github.com/rambo/TinyWire/tree/fix_rsh https://github.com/rambo/TinyWire/tree/fix_rsh should look good now, you can rename your local branch git branch -m rsh rsh_old then fetch my branch git remote add upstream https://github.com/rambo/TinyWire.git && git fetch upstream && git checkout fix_rsh then force push it on top of your old branch git push -f --set-upstream origin rsh
I think that should work... another option is that I close this PR and make a new one from that fixed branch and merge that instead :)
— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/pull/18#issuecomment-180875982.
Seems that the last command “” bombed…
561[TinyWire]$ git branch -m rsh rsh_old 562[TinyWire]$ git remote add upstream https://github.com/rambo/TinyWire.git 563[TinyWire]$ git fetch upstream remote: Counting objects: 37, done. remote: Compressing objects: 100% (4/4), done. remote: Total 37 (delta 10), reused 9 (delta 9), pack-reused 24 Unpacking objects: 100% (37/37), done. From https://github.com/rambo/TinyWire
—Scott
On Feb 6, 2016, at 5:44 PM, Scott Hartog scott@hartog.net wrote:
Hi Eero,
You're the GitHub expert. Let’s do whatever’s easier. It you need me to do something, please let me know (detailed instructions are good :) ).
I’m not sure that I didn’t mess up my branch in my attempts to squash the commits. The new commits appear to be empty, but it would be prudent for you to look it over. I have not intentionally changed anything in the last 6 or 7 hours.
—Scott
On Feb 6, 2016, at 5:40 PM, Eero af Heurlin <notifications@github.com mailto:notifications@github.com> wrote:
Yeah I reverted the previous PR, I did some rebasing and https://github.com/rambo/TinyWire/tree/fix_rsh https://github.com/rambo/TinyWire/tree/fix_rsh should look good now, you can rename your local branch git branch -m rsh rsh_old then fetch my branch git remote add upstream https://github.com/rambo/TinyWire.git https://github.com/rambo/TinyWire.git && git fetch upstream && git checkout fix_rsh then force push it on top of your old branch git push -f --set-upstream origin rsh
I think that should work... another option is that I close this PR and make a new one from that fixed branch and merge that instead :)
— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/pull/18#issuecomment-180875982.
Last command “git push -f --set-upstream origin rsh” bombed with errors, but the files in “rsh_fixed" branch seem to be correct and latest.
—Scott
On Feb 6, 2016, at 5:56 PM, Scott Hartog scott@hartog.net wrote:
Seems that the last command “” bombed…
561[TinyWire]$ git branch -m rsh rsh_old 562[TinyWire]$ git remote add upstream https://github.com/rambo/TinyWire.git https://github.com/rambo/TinyWire.git 563[TinyWire]$ git fetch upstream remote: Counting objects: 37, done. remote: Compressing objects: 100% (4/4), done. remote: Total 37 (delta 10), reused 9 (delta 9), pack-reused 24 Unpacking objects: 100% (37/37), done. From https://github.com/rambo/TinyWire https://github.com/rambo/TinyWire
- [new branch] fix_rsh -> upstream/fix_rsh
- [new branch] master -> upstream/master 564[TinyWire]$ git checkout fix_rsh Branch fix_rsh set up to track remote branch fix_rsh from upstream. Switched to a new branch 'fix_rsh' 565[TinyWire]$ git push -f --set-upstream origin rsh error: src refspec rsh does not match any. error: failed to push some refs to 'https://github.com/rshartog/TinyWire.git' https://github.com/rshartog/TinyWire.git'
—Scott
On Feb 6, 2016, at 5:44 PM, Scott Hartog <scott@hartog.net mailto:scott@hartog.net> wrote:
Hi Eero,
You're the GitHub expert. Let’s do whatever’s easier. It you need me to do something, please let me know (detailed instructions are good :) ).
I’m not sure that I didn’t mess up my branch in my attempts to squash the commits. The new commits appear to be empty, but it would be prudent for you to look it over. I have not intentionally changed anything in the last 6 or 7 hours.
—Scott
On Feb 6, 2016, at 5:40 PM, Eero af Heurlin <notifications@github.com mailto:notifications@github.com> wrote:
Yeah I reverted the previous PR, I did some rebasing and https://github.com/rambo/TinyWire/tree/fix_rsh https://github.com/rambo/TinyWire/tree/fix_rsh should look good now, you can rename your local branch git branch -m rsh rsh_old then fetch my branch git remote add upstream https://github.com/rambo/TinyWire.git https://github.com/rambo/TinyWire.git && git fetch upstream && git checkout fix_rsh then force push it on top of your old branch git push -f --set-upstream origin rsh
I think that should work... another option is that I close this PR and make a new one from that fixed branch and merge that instead :)
— Reply to this email directly or view it on GitHub https://github.com/rambo/TinyWire/pull/18#issuecomment-180875982.
Handled in #20
Summary: To allow the master to request more than one byte from the slave, the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be on line 583.
Explanation: For the current code, the ISR routine correctly sends the execution through the “USI_SLAVE_SEND_DATA” case-branch for each byte sent from the slave to the master. With the USI_REQUEST_CALLBACK() on line 613, the callback routine is called for each byte that the master requests. Assuming the callback should queue the send data for the entire request (my assumption), then the current code generates N bytes of data for each byte sent (N^2 bytes total) and only removes N bytes from the transmit buffer. This works when N==1, but if N>1 this quickly results in a transmit buffer overflow and execution hangs on line 406. (This behavior was observed by having the slave respond with the values of txHead and txTail until the slave hung on line 406.)