Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite TP/ETP and update transport layer example #391

Merged
merged 9 commits into from
Jan 18, 2024

Conversation

GwnDaan
Copy link
Member

@GwnDaan GwnDaan commented Dec 20, 2023

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

@GwnDaan GwnDaan added the iso: data link Related to the ISO-11783:3 standard label Dec 20, 2023
@GwnDaan GwnDaan requested a review from ad3154 December 20, 2023 08:45
@GwnDaan GwnDaan self-assigned this Dec 20, 2023
@GwnDaan GwnDaan force-pushed the daan/rework-extended-transport-protocol branch 3 times, most recently from 2a9bf1b to 5a32a77 Compare December 20, 2023 13:49
@GwnDaan GwnDaan changed the title [ETP]: Rewrite to match with other transport protocols [ETP]: Rewrite and update transport layer example Dec 20, 2023
@GwnDaan GwnDaan force-pushed the daan/rework-extended-transport-protocol branch from e89f6bd to 1775528 Compare December 20, 2023 20:06
@GwnDaan GwnDaan marked this pull request as ready for review December 20, 2023 20:09
@GwnDaan GwnDaan changed the base branch from daan/cleanup-transport-protocol to main January 6, 2024 14:56
@GwnDaan GwnDaan force-pushed the daan/rework-extended-transport-protocol branch from 1775528 to 82d8e23 Compare January 6, 2024 14:57
@GwnDaan GwnDaan changed the title [ETP]: Rewrite and update transport layer example Rewrite TP/ETP and update transport layer example Jan 6, 2024
@ad3154
Copy link
Member

ad3154 commented Jan 6, 2024

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.

isobus/src/can_extended_transport_protocol.cpp Outdated Show resolved Hide resolved
isobus/include/isobus/isobus/can_message_data.hpp Outdated Show resolved Hide resolved
isobus/include/isobus/isobus/can_network_configuration.hpp Outdated Show resolved Hide resolved
isobus/include/isobus/isobus/can_transport_protocol.hpp Outdated Show resolved Hide resolved
isobus/include/isobus/isobus/can_transport_protocol.hpp Outdated Show resolved Hide resolved
isobus/src/can_message_data.cpp Outdated Show resolved Hide resolved
isobus/src/can_transport_protocol_base.cpp Outdated Show resolved Hide resolved
isobus/src/can_transport_protocol_base.cpp Outdated Show resolved Hide resolved
@GwnDaan GwnDaan force-pushed the daan/rework-extended-transport-protocol branch 2 times, most recently from 06d262b to 5a4c295 Compare January 7, 2024 14:21
@GwnDaan
Copy link
Member Author

GwnDaan commented Jan 7, 2024

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

@GwnDaan GwnDaan requested a review from ad3154 January 14, 2024 06:48
@ad3154
Copy link
Member

ad3154 commented Jan 15, 2024

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 GwnDaan force-pushed the daan/rework-extended-transport-protocol branch from 14e64fd to 6ffb6c0 Compare January 15, 2024 12:37
@GwnDaan GwnDaan force-pushed the daan/rework-extended-transport-protocol branch from 6ffb6c0 to be1cb56 Compare January 15, 2024 12:39
@GwnDaan
Copy link
Member Author

GwnDaan commented Jan 15, 2024

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
Copy link
Member

ad3154 commented Jan 15, 2024

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

Copy link

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
Copy link
Member Author

GwnDaan commented Jan 16, 2024

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?

@GwnDaan GwnDaan merged commit bbbaa25 into main Jan 18, 2024
10 checks passed
@GwnDaan GwnDaan deleted the daan/rework-extended-transport-protocol branch January 18, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iso: data link Related to the ISO-11783:3 standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants