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.

Sign up for GitHub

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

Jump to bottom

Fix to allow non-flight controller heartbeats to be processed #1760

Merged
merged 4 commits into from
May 9, 2022
Merged

Fix to allow non-flight controller heartbeats to be processed #1760

merged 4 commits into from
May 9, 2022

Conversation

pweids
Copy link
Contributor

@pweids pweids commented May 3, 2022

The change from #1752 inadvertantly threw a warning for all heartbeats that came from an "INVALID" autopilot. However, this is acceptable according to the Heartbeat protocol. This small commit fixes that issue.

@pweids
Any non-flight-controller that sends a heartbeat will throw this error.
ae84d85 
This fix reverts the change, but still allows the warning for new vehicle types.
JonasVautherin
JonasVautherin previously approved these changes May 3, 2022
View reviewed changes
bazfp
bazfp requested changes May 3, 2022
View reviewed changes
Copy link
Collaborator

@bazfp bazfp left a comment

Choose a reason for hiding this comment

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

Actually, this reverts my changes and allows vehicle type to be changed by non autopilot heartbeats again, which makes no sense. Can you brace the vehicle change so it only happens if the autopilot sends it

@pweids
Copy link
Contributor Author

pweids commented May 3, 2022 via email

@pweids
Copy link
Contributor Author

pweids commented May 4, 2022

@bazfp please confirm the update fixes it

@JonasVautherin
Copy link
Collaborator

I tried it and got this kind of output:

% ./build/default/src/mavsdk_server/src/mavsdk_server udp://:14540
[02:13:42|Info ] MAVSDK version: v1.3.1-25-gea07000b (mavsdk_impl.cpp:20)
[02:13:42|Info ] Waiting to discover system on udp://:14540... (connection_initiator.h:20)
[02:13:43|Info ] New system on: 192.168.43.1:56162 (with sysid: 1) (udp_connection.cpp:192)
[02:13:43|Debug] New: System ID: 1 Comp ID: 1 (mavsdk_impl.cpp:482)
[02:13:43|Debug] Component Autopilot (1) added. (system_impl.cpp:376)
[02:13:43|Debug] Component Unsupported component (198) added. (system_impl.cpp:376)
[02:13:43|Debug] Discovered 2 component(s) (system_impl.cpp:577)
[02:13:43|Debug] Component Unsupported component (193) added. (system_impl.cpp:376)
[02:13:43|Info ] System discovered (connection_initiator.h:63)
[02:13:43|Info ] Server started (grpc_server.cpp:52)
[02:13:43|Info ] Server set to listen on 0.0.0.0:50051 (grpc_server.cpp:53)
[02:13:43|Debug] Component Camera 1 (100) added. (system_impl.cpp:376)
[02:13:43|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:43|Debug] Component Unsupported component (192) added. (system_impl.cpp:376)
[02:13:43|Debug] Component Unsupported component (191) added. (system_impl.cpp:376)
[02:13:44|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:44|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:44|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:45|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:46|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)
[02:13:46|Warn ] Vehicle type changed! New type:  Old type:  (system_impl.cpp:224)

🤔

@pweids
Copy link
Contributor Author

pweids commented May 5, 2022

I'm sure I'm missing something, but the logic that prints the error message is the same (heartbeat is from an autopilot and the vehicle type has changed). Furthermore, it's odd that the types are not printed out by the warnings. And it works just fine with you in the other version? How can I replicate your test?

See anything I'm missing?

@ManuelMeraz
Copy link

Is the intent of MAVSDK to only support autopilot types? Why would a system not want to receive non autopilot components heartbeats? For example, how would you connect to component 192, which is QGC's component ID with this implementation?

The heartbeat protocol doesn't state anything about rejecting heartbeats that aren't autopilot types. It just says to mark them like so

Autopilot type / class. Set to MAV_AUTOPILOT_INVALID for components that are not flight controllers (e.g. ground stations, gimbals, etc.).

How would a MAVSDK system connect and listen to heartbeats from these other systems?

@julianoes
Copy link
Collaborator

Is the intent of MAVSDK to only support autopilot types? Why would a system not want to receive non autopilot components heartbeats? For example, how would you connect to component 192, which is QGC's component ID with this implementation?

No, that's certainly not the intent. A System should represent all components, and they can all send heartbeats.

@julianoes
Copy link
Collaborator

I will revisit this in the next days, thanks for the PR.

@julianoes
core: fix printf formatting
85ba049
@julianoes
core: don't warn on first discovery
302e9ab 
There is no point in having a warning when first changing from the
initial MAV_TYPE_GENERIC, hence we ignore that.
@julianoes
Copy link
Collaborator

julianoes commented May 9, 2022 •
edited
Loading

I've added two commits to prevent it from warning initially, and to fix the actual warning so it shows the MAV_TYPE value properly.

@bazfp @JonasVautherin could you check if this works for you like that?

JonasVautherin
JonasVautherin approved these changes May 9, 2022
View reviewed changes
Copy link
Collaborator

@JonasVautherin JonasVautherin left a comment

Choose a reason for hiding this comment

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

Seems to be working fine for me, both on sim and on the bench 👍

@julianoes
Copy link
Collaborator

Ok, I'm going to merge it and revisit later if this doesn't work for you @bazfp.

@julianoes julianoes merged commit 01e1d55 into mavlink:main May 9, 2022
@bazfp
Copy link
Collaborator

bazfp commented May 10, 2022

This looks like it works for me. The intent of the original change was to stop rogue MAVLink components incorrectly setting the entire system vehicle type for MAVSDK. It was my mistake that it blocked them :)

@pweids
Copy link
Contributor Author

pweids commented May 10, 2022

Thanks, everyone!

bazfp
bazfp reviewed Jun 8, 2022
View reviewed changes
src/mavsdk/core/system_impl.cpp
} else {
const auto new_vehicle_type = static_cast<MAV_TYPE>(heartbeat.type);
if (heartbeat.autopilot != MAV_AUTOPILOT_INVALID && _vehicle_type != new_vehicle_type &&
_vehicle_type != MAV_TYPE_GENERIC) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is a mistake, should this read new_vehicle_type != MAV_TYPE_GENERIC instead? Otherwise MAVSDK will forever be stuck as generic type.

bazfp
bazfp reviewed Jun 8, 2022
View reviewed changes
Copy link
Collaborator

@bazfp bazfp left a comment

Choose a reason for hiding this comment

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

Just ran into some issues while testing this:

@julianoes
Copy link
Collaborator

@bazfp ok what about it? 🤔

@bazfp bazfp mentioned this pull request Jun 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bazfp bazfp bazfp requested changes

@julianoes julianoes julianoes approved these changes

@JonasVautherin JonasVautherin JonasVautherin approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pweids @JonasVautherin @ManuelMeraz @julianoes @bazfp