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

Accept onboard computer heartbeats as telemetry #10194

Closed
wants to merge 3 commits into from

Conversation

okalachev
Copy link
Contributor

@okalachev okalachev commented Aug 8, 2018

This PR solves #10042 problem, that STATUSTEXT messages are not passed, when there is no GCS.
Also, this is a partial implementation of #7985.

I pass telemetry_status, when OBC heartbeats received, but I changed the commander logic, for not considering OBC as a datalink.

So NAV_DLL_ACT, COM_DL_LOSS_T, COM_DL_REG_T are only about GCS link, not OBC (not renaming for backwards compatibility).

The OBC failsafe params, like NAV_OBC_DLL_ACT, COM_OBC_DL_LOSS_T, COM_OBC_DL_REG_T can be added later.

@LorenzMeier , @TSC21 .

@TSC21
Copy link
Member

TSC21 commented Aug 8, 2018

This looks reasonable and can be extended for an OBC datalink failsafe after. Can you please rebase?

@okalachev okalachev changed the title Obc heartbeats Accept onboard computer heartbeats as telemety Aug 8, 2018
@okalachev
Copy link
Contributor Author

@TSC21 , thanks, I'll look.

@okalachev okalachev changed the title Accept onboard computer heartbeats as telemety Accept onboard computer heartbeats as telemetry Aug 8, 2018
if (msg->sysid != mavlink_system.sysid && hb.type == MAV_TYPE_GCS) {

/* accept only heartbeats from GCS or Onboard computers */
if (hb.type == MAV_TYPE_GCS || hb.type == MAV_TYPE_ONBOARD_CONTROLLER) {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason we can't drop this check entirely now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if there is a heartbeat from another drone (in a swarm network), charging station, or any other device? Should it be considered as a telemetry link?
I don't think so.

@@ -56,7 +56,7 @@ static constexpr const char reason_no_offboard[] = "no offboard";
static constexpr const char reason_no_rc_and_no_offboard[] = "no RC and no offboard";
static constexpr const char reason_no_local_position[] = "no local position";
static constexpr const char reason_no_global_position[] = "no global position";
static constexpr const char reason_no_datalink[] = "no datalink";
static constexpr const char reason_no_datalink[] = "no GCS datalink";
Copy link
Member

Choose a reason for hiding this comment

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

How about ground control instead of GCS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a huge difference, but I can change it.

@dagar
Copy link
Member

dagar commented Aug 8, 2018

A few minor comments, but overall looks good.

@okalachev
Copy link
Contributor Author

okalachev commented Aug 9, 2018

@TSC21 , I found the bug.

The problem starts, when multiple telemetry "sources" are on the same Mavlink instance, so this is the same telemetry_status topic instance, and the same "telemetry" in commander's telemetry_data array.

So, I don's see an easy solution yet, looks like a lot of telemetry logic should be changed.

@TSC21
Copy link
Member

TSC21 commented Aug 9, 2018

@okalachev that probably means we need a different telemetry_status topic instance for each of the telemetry sources. Probably multitopic is required.

@okalachev
Copy link
Contributor Author

I guess the maximum number of topic instances are ORB_MULTI_MAX_INSTANCES, which is 4. But the number of telemetry "sources" that way would be something like 255 (systems) * 255 (systems' components).

So, maybe not to use multitopic for that. Instead, to add system and component ids to telemetry_status?

@TSC21
Copy link
Member

TSC21 commented Aug 9, 2018

I guess the maximum number of topic instances are ORB_MULTI_MAX_INSTANCES, which is 4. But the number of telemetry "sources" that way would be something like 255 (systems) * 255 (systems' components).

Well I was thinking on limit the telemetry sources based on type and not on sys+comp id. But I guess that would make more sense yes. Can you propose that in a new issue as a RFC? Probably better to have a discussion about it first before one commits to it.

@okalachev
Copy link
Contributor Author

You mean to write an RFC for adding sys and comp ids to telemetry_status?

I'm not sure this is so large change to make an RFC.

@TSC21
Copy link
Member

TSC21 commented Aug 9, 2018

I'm not sure this is so large change to make an RFC.

It's a conceptual/structural change/feature that needs to be discussed.

@LorenzMeier
Copy link
Member

Could you please rebase? Thanks!

@okalachev
Copy link
Contributor Author

Unfortunately, it doesn't make sense for now, because this change introduces a new bug, that I described in previous messages.

I don't see an easy solution yet, without rewriting telemetry_status logic.

@TSC21
Copy link
Member

TSC21 commented Nov 25, 2018

@okalachev what's the status of this? Have you thought of a way of bringing this into a state that does work?

@okalachev
Copy link
Contributor Author

@TSC21, sorry, unfortunately I haven't worked at this yet. The easy workaround is to send fake GCS heartbeats from ROS code. The conversation can be continued at #10042.

This PR can be closed I guess, as the implementation is incorrect.

@TSC21
Copy link
Member

TSC21 commented Nov 25, 2018

The PR is not totally incorrect. It's rather incomplete. But I accept that we can bring a more complete solution in a new PR. Please fill free to close it or maybe we can iterate from here.

@dagar
Copy link
Member

dagar commented Nov 28, 2018

Revisiting this idea - #10933

@stale
Copy link

stale bot commented Feb 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Mar 12, 2019

Closing as stale.

@stale stale bot closed this Mar 12, 2019
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