Change to robustly support multi-byte requestFrom() calls in the TinyWireS/usiTwiSlave library. #18
Conversation
Summary: To allow the master to request more than one byte from the slave in a requestFrom() call, the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be moved to 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, 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.) If you move USI_REQUEST_CALLBACK() to line 583, then the callback is called only once per master request (when the request is initially received). One other related note: As written, the useable size of the rxBuf is actually TWI_RX_BUFFER_SIZE - 1. Same for txBuf/TWI_TX_BUFFER_SIZE. This is because the Head pointer cannot fully wrap back to the value of the Tail pointer. (Doing so would appear to be an empty buffer.) This detail might be important to someone who thinks they can use the entire buffer.
This file
|
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, |
|
Merged earlier PR and now we have conflicts. Try The latter line will update your local clones master branch from the upstream, then you can do |
|
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
|
Summary: To allow the master to request more than one byte from the slave in a requestFrom() call, the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be moved to 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, 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.) If you move USI_REQUEST_CALLBACK() to line 583, then the callback is called only once per master request (when the request is initially received). One other related note: As written, the useable size of the rxBuf is actually TWI_RX_BUFFER_SIZE - 1. Same for txBuf/TWI_TX_BUFFER_SIZE. This is because the Head pointer cannot fully wrap back to the value of the Tail pointer. (Doing so would appear to be an empty buffer.) This detail might be important to someone who thinks they can use the entire buffer. Create TinyWireS_Stress_Master.ino Delete TinyWireS_Stress_Master.ino Create TinyWireS_Stress_Master.ino This file Create TinyWireS_Stress_Slave.ino Update TinyWireS_Stress_Master.ino Update TinyWireS_Stress_Slave.ino
Squashing multiple commits into a single commit.
Create TinyWireS_Stress_Master.ino Create TinyWireS_Stress_Slave.ino Summary: To allow the master to request more than one byte from the slave in a requestFrom() call, the USI_REQUEST_CALLBACK() call on line 613 of the current usiTwiSlave.c code should be moved to 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, 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.) If you move USI_REQUEST_CALLBACK() to line 583, then the callback is called only once per master request (when the request is initially received). One other related note: As written, the useable size of the rxBuf is actually TWI_RX_BUFFER_SIZE - 1. Same for txBuf/TWI_TX_BUFFER_SIZE. This is because the Head pointer cannot fully wrap back to the value of the Tail pointer. (Doing so would appear to be an empty buffer.) This detail might be important to someone who thinks they can use the entire buffer.
|
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 sort out squash conflicts >vi TinyWireS/examples/TinyWireS_Stress_Master/TinyWireS_Stress_Master.ino sort out squash conflictsvi TinyWireS/examples/TinyWireS_Stress_Slave/TinyWireS_Stress_Slave.ino Thanks,
|
|
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 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
|
|
Seems that the last command “” bombed… 561[TinyWire]$ git branch -m rsh rsh_old
—Scott
|
|
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
|
|
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.)