Open-Agriculture / AgIsoStack-plus-plus

AgIsoStack++ is the completely free open-source C++ ISOBUS library for everyone
https://agisostack.com/
MIT License
168 stars 40 forks source link

Rewrite TP/ETP and update transport layer example #391

Closed GwnDaan closed 5 months ago

GwnDaan commented 6 months ago

This PR includes a full rewrite of the extended transport protocol where I've taken the rewrite of the TP as base, and also a freshing transport layer example that shows of the complete transport layer - with progress indication! :)

Screencast from 20-12-23 21:17:27.webm

ad3154 commented 6 months ago

I did try testing our VT example against our VT, and got a crash in the VT example here:

image

Buffer size was 0

Callstack:

    VT3ExampleTarget.exe!std::vector<unsigned char,std::allocator<unsigned char>>::operator[](const unsigned __int64 _Pos) Line 1893    C++
    VT3ExampleTarget.exe!isobus::CANMessageDataCallback::get_byte(unsigned __int64 index) Line 107  C++
    VT3ExampleTarget.exe!isobus::ExtendedTransportProtocolManager::send_data_transfer_packets(std::shared_ptr<isobus::ExtendedTransportProtocolManager::ExtendedTransportProtocolSession> & session) Line 596   C++
    VT3ExampleTarget.exe!isobus::ExtendedTransportProtocolManager::update_state_machine(std::shared_ptr<isobus::ExtendedTransportProtocolManager::ExtendedTransportProtocolSession> & session) Line 696 C++
    VT3ExampleTarget.exe!isobus::ExtendedTransportProtocolManager::update() Line 571    C++
    VT3ExampleTarget.exe!isobus::CANNetworkManager::update() Line 249   C++
    VT3ExampleTarget.exe!isobus::periodic_update_from_hardware() Line 304   C++
    VT3ExampleTarget.exe!isobus::CANHardwareInterface::update_thread_function() Line 297    C++

image

So this would have been when trying to upload the object pool, and trying to use the data callback method of transferring the data.

GwnDaan commented 6 months ago

Good catch! I fixed that issue with the callback, and resolved your comments. Plus fixed another issue where the protocols would listen (and respond) to messages not destined to them

ad3154 commented 5 months ago

There is still some callback issue.... when I run the VT example, I can see situations where the callback gets called with a buffer of size 0, so when the callback tries to memcpy the data into it, that probably doesn't do anythign, and the end result later is an exception when we try to index into a 0 size buffer. This was seen in can_message_data.cpp line 113

image

GwnDaan commented 5 months ago

buffer of size 0

Ahh my bad, I accidentally used reserve on the buffer instead of actually resizing the buffer. GCC didn't complain about it, but it was indeed not correct. It is fixed now

ad3154 commented 5 months ago

Not crashing anymore with the VT or seeder example which is good. TP broadcast data message timing seems a bit long, like usually > 60ms which is curious but not super concerning.

Transport layer example needs a few updates to display 0-100 again.

image

sonarcloud[bot] commented 5 months ago

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

47 New issues
0 Security Hotspots
44.8% Coverage on New Code
2.9% Duplication on New Code

See analysis details on SonarCloud

GwnDaan commented 5 months ago

TP broadcast data message timing seems a bit long, like usually > 60ms which is curious but not super concerning.

Hmm that's weird, in the unit test BroadcastMessageSending it seems to think it is sending the frames within +/- 50ms: image

For me with vcan it seems to be pretty consistent 52ms inbetween frames (according to candump) image

Considering we are using a 4ms update loop by default, that seems okay: image

I make a guess that you used a lower tick-rate?