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

generator: mavlink_channel_t with correct length #473

Merged
merged 2 commits into from Nov 25, 2020

Conversation

julianoes
Copy link
Contributor

For cases where more than 4 mavlink channels are required, the mavlink_channel_t does not match. Therefore, I suggest that the enum needs to be provided separately if the default channel number is not used.

For cases where more than 4 mavlink channels are required, the
mavlink_channel_t does not match. Therefore, I suggest that the enum
needs to be provided separately if the default channel number is not
used.
@len0rd
Copy link
Contributor

len0rd commented Nov 3, 2020

Hey, nice work! This is similar to a PR I opened up a few months ago in response to an issue I made about this ( #404 ). Your change is more along the lines of the fix I was thinking when I made my PR originally.

Unfortunately, to preserve backwards compatibility, it may be necessary to maintain the default definition of mavlink_channel_t somehow. (Perhaps some type of ifndef wrapper similar to my PR)?

@julianoes
Copy link
Contributor Author

Thanks for the comment @len0rd. I would be happy with either PR.

@hamishwillee
Copy link
Contributor

@peterbarker Could you please review this?

@hamishwillee
Copy link
Contributor

@peterbarker ?

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what issue this is solving. Are you really trying to use MAVLINK_COMM_7?
Maybe we should add
#define MAVLINK_COMM_N(n) mavlink_channel_t(MAVLINK_COMM_0+(n))
or we could add:
#ifndef HAVE_MAVLINK_CHANNEL_T
around the enum define, to allow it to be overridden

generator/C/include_v2.0/mavlink_types.h Show resolved Hide resolved
@julianoes
Copy link
Contributor Author

julianoes commented Nov 11, 2020

@tridge thanks for your answer. You're right that it would break existing users that define MAVLINK_COMM_NUM_BUFFERS.

Maybe we should add
#define MAVLINK_COMM_N(n) mavlink_channel_t(MAVLINK_COMM_0+(n))
or we could add:
#ifndef HAVE_MAVLINK_CHANNEL_T

What about doing both?

This allows to define mavlink_channel_t to match the
MAVLINK_COMM_NUM_BUFFERS chosen.
@julianoes
Copy link
Contributor Author

@tridge ok, I've added ifndef HAVE_MAVLINK_CHANNEL_T as you suggested. That should work, and is basically what @len0rd suggested in #426.

@julianoes
Copy link
Contributor Author

@tridge could you check this again please? Thanks!

@hamishwillee
Copy link
Contributor

Hi @tridge
Can you please re-review?

@tridge tridge merged commit 1dfd28b into ArduPilot:master Nov 25, 2020
@julianoes
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants