-
Notifications
You must be signed in to change notification settings - Fork 93
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
Conversation
26388ef
to
b981339
Compare
b981339
to
3ea04cd
Compare
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. |
@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 ? |
3ea04cd
to
caffa39
Compare
@julianoes would you have time to review it so that I can integrate the functionality further away ? |
14bff0c
to
9fde2c4
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
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. |
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 |
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. |
9fde2c4
to
0f22334
Compare
@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 ? |
0f22334
to
1ecb8fe
Compare
Hello team MAVSDK Would it be possible to merge this ? Or what is the release policy for MAVSKD proto ? |
@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. |
1ecb8fe
to
dc46553
Compare
@julianoes work in progress here Sorry for the delay, I needed some time to have clean version of MAVSDK, etc |
dc46553
to
621e0e6
Compare
621e0e6
to
85ce204
Compare
Co-Authored-By: Julian Oes <[email protected]>
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.
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.