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

review PX4 Mavlink module write helper (send_bytes) #10389

Open
dagar opened this issue Sep 1, 2018 · 7 comments
Open

review PX4 Mavlink module write helper (send_bytes) #10389

dagar opened this issue Sep 1, 2018 · 7 comments
Assignees

Comments

@dagar
Copy link
Member

dagar commented Sep 1, 2018

Each mavlink packet is broken up into several writes (header, payload, optional signature). The PX4 mavlink module write helper (send_bytes) checks the remaining buffer on each of these writes and aborts with a logged tx error if there isn't sufficient space.

https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_main.cpp#L942-L994

The result is a garbage partially written mavlink packet and potentially lost data.

I propose we do this check per mavlink message send initially.

@LorenzMeier
Copy link
Member

I need to cross check. It certainly was whole packet size earlier and got broken in the process if it really is broken.

@acfloria
Copy link
Member

acfloria commented Sep 1, 2018

I think the messages are splitted. I noticed it when I was writing the IridiumSBD driver. There I am checking if the whole package fits in the buffer and reset that one if it doesn't.

If we are rewriting how packages are sent it would be great if we can get some feedback if the bytes were sent successfully. Currently if one is using mavlink_msg_MESSAGE_send_struct there is no feedback if the write is successful. This would be especially helpful for example for low latency telemetry where one wants to make sure that the messages which were scheduled to sent are actually sent.

@dagar
Copy link
Member Author

dagar commented Sep 1, 2018

https://github.com/mavlink/c_library_v2/blob/master/mavlink_helpers.h#L349

It can be fixed in the mavlink library, but I'd also like to preserve the data and skip the unnecessary work px4 mavlink module side.

@dagar dagar added the bug label Sep 1, 2018
@dagar dagar added this to the Release v2.0.0 milestone Sep 1, 2018
dagar added a commit that referenced this issue Sep 2, 2018
@dagar
Copy link
Member Author

dagar commented Sep 2, 2018

PR for testing #10394

dagar added a commit that referenced this issue Sep 16, 2018
dagar added a commit that referenced this issue Nov 9, 2018
dagar added a commit that referenced this issue Nov 16, 2018
@stale stale bot added the Admin: Wont fix label Feb 4, 2019
@julianoes
Copy link
Contributor

@dagar should we test this?

@dagar
Copy link
Member Author

dagar commented Feb 4, 2019

@julianoes yes, there are a number of small mavlink issues like this we should discuss.

@PX4 PX4 deleted a comment from stale bot Jun 24, 2019
@stale stale bot removed the Admin: Wont fix label Jun 24, 2019
@PX4 PX4 deleted a comment from stale bot Jun 24, 2019
@PX4 PX4 deleted a comment from stale bot Sep 24, 2019
@stale stale bot removed the Admin: Wont fix label Sep 24, 2019
@stale
Copy link

stale bot commented Dec 23, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants