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

PR Add battery proto status message for v1 #297

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

Katawann
Copy link
Contributor

I am interested in receiving more information about the battery status through MAVSDK. Mainly I need the current consumption, but I took the opportunity to add the other information.

  • temperature
  • current_battery
  • current_consumed
  • energy_consumed

I don't see the temperature and energy_consumed in my flight logs so not sure if it is relevant to add them. I also saw that there was already a PR in progress for adding messages for the v2 battery but nothing done for v1 from what I found.

protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
protos/telemetry/telemetry.proto Outdated Show resolved Hide resolved
@hamishwillee
Copy link

There's only one battery API from a MAVSDK perspective right? If it were me I would make that match the MAVLink v2 in terms of units and naming, where possible. Most of it maps pretty cleanly - its really only the faults and status that are in discussion.

@Katawann
Copy link
Contributor Author

Katawann commented Sep 14, 2022

@hamishwillee I saw your proposition for new battery status v2 but you created a new function v2 so I thought it was you using another mavlink message. So I can bring the modifications you did in #290 without the status information so that I can make it works with v1. What do you think ?

@Katawann Katawann force-pushed the pr-battery-status-v1 branch from 3ea04cd to caffa39 Compare September 14, 2022 16:38
@Katawann Katawann requested a review from julianoes September 16, 2022 12:56
@Katawann
Copy link
Contributor Author

@julianoes would you have time to review it so that I can integrate the functionality further away ?

@Katawann Katawann force-pushed the pr-battery-status-v1 branch from 14bff0c to 9fde2c4 Compare September 27, 2022 20:17
julianoes
julianoes previously approved these changes Sep 27, 2022
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

This is good for me. @hamishwillee do you want to veto this or are you ok with it? I don't understand all the things going on with battery, so I'm asking you.

hamishwillee
hamishwillee previously approved these changes Oct 4, 2022
Copy link

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

@julianoes I'm OK with this. I've added some comments in case you would prefer to match MAVLink - i.e. MAVLink sends mA rather than A for efficiency reasons, but I am not sure whether consumers care.

I think the new battery message will be coming out in the next couple of days. We're in a final broader-round review stage. The main additions that will come from that will be the status fields.
The most interesting change is covered in https://github.com/mavlink/MAVSDK-Proto/pull/297/files#r987400519 - but I think that if you leave things exactly as they are you can then simply extend the MAVSDK with capacity consumed and capacity remaining in mAh that are populated only when the relative to full flag is set.

@julianoes
Copy link
Collaborator

So what do we do to this MAVSDK API once the new message is out? I would like not to carry two different messages. The point of MAVSDK is to avoid that sort of mess.

@Katawann
Copy link
Contributor Author

I think the new battery message will be coming out in the next couple of days. We're in a final broader-round review stage. The main additions that will come from that will be the status fields.

If @hamishwillee is close to get something with v2 I guess it's worth waiting the end of the review stage to ensure we have similar messages for non smart battery communication

@hamishwillee
Copy link

The latest version of the battery_status_v2 just went into development.xml. This is compatible/good. If it were me I'd remove energy consumed because anecdotally it is rarely reported by a GCS - and it definitely won't be sent in the new battery message.

If you think this can be derived from the new message then sure - but not if it can't.

Upshot, feel free to merge.

@Katawann Katawann dismissed stale reviews from hamishwillee and julianoes via 0f22334 November 1, 2022 07:15
@Katawann Katawann force-pushed the pr-battery-status-v1 branch from 9fde2c4 to 0f22334 Compare November 1, 2022 07:15
@Katawann
Copy link
Contributor Author

Katawann commented Nov 1, 2022

@julianoes I removed the energy consumed as suggested by @hamishwillee and I matched order and name based on what have been done for the battery status v2. Could check it and merged if it's okay ?

@Katawann Katawann requested a review from julianoes November 1, 2022 07:18
@Katawann Katawann force-pushed the pr-battery-status-v1 branch from 0f22334 to 1ecb8fe Compare November 2, 2022 20:09
julianoes
julianoes previously approved these changes Nov 2, 2022
@Katawann
Copy link
Contributor Author

Katawann commented Nov 22, 2022

Hello team MAVSDK

Would it be possible to merge this ? Or what is the release policy for MAVSKD proto ?

@julianoes
Copy link
Collaborator

@Katawann have you already done the MAVSDK part? If so, you can push it and make a PR. That way we know it's ready and once this is merged, the MAVSDK PR will follow right after.

@Katawann
Copy link
Contributor Author

Katawann commented Dec 1, 2022

@julianoes work in progress here

Sorry for the delay, I needed some time to have clean version of MAVSDK, etc

@Katawann Katawann force-pushed the pr-battery-status-v1 branch from dc46553 to 621e0e6 Compare December 1, 2022 11:26
julianoes
julianoes previously approved these changes Dec 3, 2022
@julianoes julianoes merged commit 1490189 into mavlink:main Jan 12, 2023
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.

4 participants