bipropellant / bipropellant-protocol

defines and implements a low level protocol shared between a few different hoverboard repositories
MIT License
20 stars 17 forks source link

no-ACK SOM #10

Closed p-h-a-i-l closed 5 years ago

p-h-a-i-l commented 5 years ago

see https://github.com/bipropellant/hbprotocol/issues/4 (https://github.com/orgs/bipropellant/projects/1#card-21024770)

Implement additional SOM to indicate Messages, which do not need to be acknowledged. Useful for information which is very time sensitive, like pmw set point and sensor data, where buffering during communication errors is not useful. Reduces load on UART, Messages can also be sent quickly, even for some reason ACKs are not being sent or only RX is disturbed.

Allows very simple implementations, where one side can only send messages, but can not parse the replies.

p-h-a-i-l commented 5 years ago

Don't merge yet, not tested.

btsimonh commented 5 years ago

one comment: char tmp[] = { PROTOCOL_SOM_NOACK, CI, 1, PROTOCOL_CMD_NACK, 0 }; I think ACK and NACK should send with the normal SOM, as they are (only?) part of that 'stream' of messages? - and also this would be a breaking change.... I see where you are thinking, but ACK/NACK is dealt with at a low level..... Apart from that, all in the right direction.

p-h-a-i-l commented 5 years ago

I think ACK and NACK should send with the normal SOM, as they are (only?) part of that 'stream' of messages? - and also this would be a breaking change.... I see where you are thinking, but ACK/NACK is dealt with at a low level.....

My thoughts were that neither ACK or NACK are being confirmed, so they should get the NoACK SOM. But being a breaking change is also true. On the other hand - we changed so much in the last days ;)

p-h-a-i-l commented 5 years ago

tested in context of https://github.com/bipropellant/hbprotocol/pull/12

btsimonh commented 5 years ago

ok, pulled branches down and now looking at combined 'subscription' branch. The CI handling need to be separate for ACK and NOACK, else things will get really screwy. Suggest storing lastCI and stats for each 'stream'? and maybe a combined stats? Still want ACK/NACK to have SOM 02 :). If only so that existing driving code does not need adjusting immediately. Take a look at subscribe_CI branch?

Subscribe should have a pre-write to memset structure to zero in case of a short write... added to subscribe_CI, with NR flow.

Updated/Cleaned up subscribe NR flow again.

p-h-a-i-l commented 5 years ago

The CI handling need to be separate for ACK and NOACK, else things will get really screwy.

yeah, Will be more clean

Suggest storing lastCI and stats for each 'stream'? and maybe a combined stats?

Combining is not important for me. When I read them, it's very simple to add.

Still want ACK/NACK to have SOM 02 :). If only so that existing driving code does not need adjusting immediately.

Ok :)

Take a look at subscribe_CI branch? Subscribe should have a pre-write to memset structure to zero in case of a short write... added to subscribe_CI, with NR flow.

Looks good, but didn't take a deep dive. Did you test?

btsimonh commented 5 years ago

yep; running now. results of subscribing (but my receipt of subscribed hall & xyt causes reset message to be sent too from my end :). ):

Hall L: 0 Hall R: 5 Deadreckoning X: -25 Deadreckoning Y: 56 Deadreckoning t: 1 Msgs: 1238 MsgsNOACK: 1238 CIErrs: 11 ACKs: 1232 NACKs: 10 CIErrsNOACK: 5 CSErrs: 0 so ~1% NACKs for me on software serial, 0.5 % CI error in NOACK messages. Subscription period = 1024 for hall and xyt. each received causes a msg to the hoverboard which is then acked as well. So quite busy.... Seems no receipt character loss, as no CS errors? So CIErrsNOACK must be discarded messages? surely can't be buffer full... NACKS indicate receipt errors on hoverboard - maybe software serial receipt is not perfect.... s