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

Add status topic #406

Merged
merged 5 commits into from
Jan 27, 2024
Merged

Add status topic #406

merged 5 commits into from
Jan 27, 2024

Conversation

whoenig
Copy link

@whoenig whoenig commented Jan 19, 2024

No description provided.

@whoenig
Copy link
Author

whoenig commented Jan 19, 2024

This is related to #149.

In addition, it allows to look at the connection quality for both unicast and broadcast, for firmware versions that have bitcraze/crazyflie-firmware#1341 applied.

@whoenig whoenig marked this pull request as ready for review January 19, 2024 14:56
@knmcguire
Copy link
Collaborator

Hi! So we need to get the Crazyflie PR merged first for this though (hopefully soon).

Usually with the cflib link, I try to wait until the firmware release is out before merging new stuff for stability. Is that perhaps we can consider for the cpp link as well? I do see that you have some backwards compatibility functionality in there already so that might be enough as well.

@whoenig
Copy link
Author

whoenig commented Jan 22, 2024

I tested with old and "new" firmware, so I think it's safe to be merged. On older firmware versions, there will be just a warning printed and the numbers remain 0.

@knmcguire
Copy link
Collaborator

Gotcha. bitcraze/crazyflie-firmware#1341 has been merged and I will now review this PR

Copy link
Collaborator

@knmcguire knmcguire left a comment

Choose a reason for hiding this comment

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

For the rest it looks good to me, but I didn't check it too much on c++ accuracy, however it seems to build in your case so that seems okay.

// general status
uint16_t supervisorInfo; // supervisor.info
// battery related
// TODO: would it be better to use pm.batteryLevel?
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the sack of 'easier understandability' I would say yes, but it might send the wrong message. Since there is no linear relationship between how fast discharging on the crazyflie or bigger bolt/BQ devices this might be tricky.... on the other hand, perhaps that won't be that much of a problem if the user is aware of this.

Copy link
Author

Choose a reason for hiding this comment

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

What would the proper bolt support look like? We would either have "batteryLevel" or vbat and extvbat? Was there a reason why "pm.vbat" doesn't "point to" the external value on a bolt-based build?

Choose a reason for hiding this comment

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

Using BQ-deck one can actually have two batteries at the same time. On the Bolt this is also in some sense true but maybe not practical. It makes it a bit complicated and perhaps having one variable representing the "flying" battery level would be preferred.

@whoenig whoenig merged commit 5d8cbc4 into main Jan 27, 2024
3 checks passed
@whoenig whoenig deleted the feature_connection_stats_broadcast branch January 27, 2024 13:43
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.

3 participants