SerialTransfer.bytesRead != SerialTransfer.packet.bytesRead when using callbacks
PaulBouchier opened this issue · 3 comments
Describe the bug
The example arduino sketch in pySerialTransfer/README.md does a loopback echo using this line:
for(uint16_t i=0; i < myTransfer.bytesRead; i++)
which leads me to think I can rely on myTransfer.bytesRead to tell how many bytes were read during myTransfer.Available(). However, that is incorrect when using Arduino callbacks, because Arduino callbacks are called from Packet.parse() inside of SerialTransfer.available(), and since Packet.parse() has not returned bytesRead, SerialTransfer.bytesRead still contains 0, because as far as it's concerned, the read has not finished yet.
To Reproduce
Run the attached two programs: main.cpp on Arduino, loopback_callback.py on a linux (or any) PC.
Observe the following output from Arduino:
SerialTransfer Loopback Test
In echo_cb
packet bytesRead: 40 SerialTransfer bytesRead: 0 packet status 2
Got a message ID: 0 sending back msg len - one of the following two values: 0 40
myTransfer.tick() received a message
In list_cb
Got a message ID: 1 sending back msg len - one of the following two values: 0 8
myTransfer.tick() received a message
Observe in main.cpp that handleRxMsg is called from the two callbacks echo_cb() and list_cb(), and line 41 prints the message above that it is sending back msg len - one of the following two values, and it lists 0 and 40, that being SerialTransfer.bytesRead and SerialTransfer.packet.bytesRead. Code supplied uses myTransfer.bytesRead per the example cited above, but if you uncomment the lines at 50, 51 and 54, and comment 47, 48 and 53 the program works, because it will then use myTransfer.packet.bytesRead.
Expected behavior
Two variables, both called bytesRead, should contain the same value. Or better yet, there should not be two variables - duplication of data is always bad and this is a case where it broke the code owing to inconsistency. Or maybe you should make more class internal variables private - it's pretty uncool allowing users to get in & mess with internal class variables. Not sure of the best solution here, but the environment that callbacks are called in is significantly different than the non-callback case - maybe that environment needs documenting.
Screenshots
If applicable, add screenshots to help explain your problem.
Desktop (please complete the following information):
Ubuntu 20.04
Smartphone (please complete the following information):
- Device: [e.g. iPhone6]
- OS: [e.g. iOS8.1]
- Browser [e.g. stock browser, safari]
- Version [e.g. 22]
Additional context
The python file is provided as loopback_callback_copy.py.txt and main.cpp also has a .txt extension, to enable uploading.
Is this still an issue in light of PowerBroker2/pySerialTransfer#50?
Also, I added callback functionality for completeness, but it might be more stable to process the packets normally (w/o callbacks).
I think this is a documentation issue. I'm thinking a note in the README.md along the lines of "Note: when using callbacks on Arduino, the following SerialTransfer variables are invalid: SerialRead, & any others". Callbacks are really important functionality, and I greatly value it, because it means a sender can send a message and the right handler will get called on the other side to handle that message type. It helps encapsulation of message flows. If you want I'll propose some documentation.
I don't think this issue of proper callback use is related to PowerBroker2/pySerialTransfer#50 which is fundamentally about use of different types on the Arduino side vs. python size (float vs. double).
Documentation updated in pull request #84