PowerBroker2 / SerialTransfer

Arduino library to transfer dynamic, packetized data fast and reliably via Serial, I2C, or SPI
MIT License
427 stars 68 forks source link

SerialTransfer.bytesRead != SerialTransfer.packet.bytesRead when using callbacks #83

Open PaulBouchier opened 2 years ago

PaulBouchier commented 2 years ago

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):

Additional context The python file is provided as loopback_callback_copy.py.txt and main.cpp also has a .txt extension, to enable uploading.

loopback_callback (copy).py.txt ext** main (copy).cpp.txt

PowerBroker2 commented 2 years ago

Is this still an issue in light of https://github.com/PowerBroker2/pySerialTransfer/issues/50?

Also, I added callback functionality for completeness, but it might be more stable to process the packets normally (w/o callbacks).

PaulBouchier commented 2 years ago

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 https://github.com/PowerBroker2/pySerialTransfer/issues/50 which is fundamentally about use of different types on the Arduino side vs. python size (float vs. double).

PaulBouchier commented 2 years ago

Documentation updated in pull request #84