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

mavlink: use command to set SiK ID #21082

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

mavlink: use command to set SiK ID #21082

wants to merge 3 commits into from

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Feb 8, 2023

This removes the param MAV_SIK_RADIO_ID and replaces it with a MAVLink command to set the radio's net ID.

Using a command has the benefit that we get feedback whether the change has been accepted. The code now also reads back the bytes after doing the config in order to check for the radio's OK feedback.

Previously, the commands were just sent blindly and there is the chance they did not actually get through.

This replaces #20984.

The command is pending to be included in MAVLink here:
mavlink/mavlink#1948

An example to trigger this from the ground implemented in MAVSDK:
mavlink/MAVSDK#1983

This removes the param MAV_SIK_RADIO_ID and replaces it with a MAVLink
command to set the radio's net ID.

Using a command has the benefit that we get feedback whether the change
has been accepted. The code now also reads back the bytes after doing
the config in order to check for the radio's OK feedback.

Previously, the commands were just sent blindly and there is the chance
they did not actually get through.

Signed-off-by: Julian Oes <[email protected]>
@dagar
Copy link
Member

dagar commented Feb 8, 2023

Doesn't it seem potentially problematic in lots of situations to be configuring your mavlink radio over mavlink? I believe there's a variation on the command you can do that makes the change on any remote radio as well.

I have no objection to the command, but I would still keep a convenient local mechanism. How about either the questionable MAV_SIK_RADIO_ID or mavlink_main cli helper that uses the new command?

switch (_parse_state) {
case OkParseState::None:
// Nothing to do.
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well break from the for loop here too, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!

@julianoes
Copy link
Contributor Author

julianoes commented Feb 8, 2023

I can try to review re-add the param.

@dagar
Copy link
Member

dagar commented Feb 8, 2023

I can try to review the param.

Thanks, this on top of #20984 could be fine if it fits.

However, this is now without the functionality to reset to factory
default.

Signed-off-by: Julian Oes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants