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

MAV_CMD_BATTERY_RESET move to common #1897

Closed
wants to merge 1 commit into from
Closed

Conversation

hamishwillee
Copy link
Collaborator

This is a proposal to move the MAV_CMD_BATTERY_RESET command in the ArduPilot dialect to common.xml.
It is dependent on someone in PX4 agreeing this is useful and they would like to implement it.


Power modules are powered on boot and start measuring the consumed power. The assumption is normally that the battery is full on boot, and the remaining power will be the full battery level minus the consumed power.

This works (provided the battery is full on boot) for a single battery. However when you hotswap a battery with a full battery the values are all immediately wrong because nothing has been consumed of the new battery.

The MAV_CMD_BATTERY_RESET command in the ArduPilot dialect allows a GCS to set the remaining percentage of the battery. So you could immediately set the value back to 100%.
The command should be ignored by a smart battery, which knows its battery level and reports it accurately.

NOTE: I am not sure how the autopilot would actually manage this. It might just immediately set the original capacity to be doubled (or whatever value is appropriate for the setting). Or it might assume a battery swap, and store the current consumed current as an offset.

The command is also used to reset fuel-based power systems, which are often set to less than 100%.

@hamishwillee
Copy link
Collaborator Author

This fixes a hole with power module based battery level tracking. This is a "discussion" PR. My thoughts:

  • "Battery" is an overly specific name for something that can be used for any kind of fuel based system.
  • This allows up to 7 batteries to be reset - will we ever need more? I guess if so we could add an extra bitmask.
  • Not sure PX4 will be interested. Power modules are supported, but the main audience are moving more towards smart batteries.
  • We should make it clear how it behaves - i.e. does calling this assume a battery swap and treat power module values as an offset.

@hamishwillee
Copy link
Collaborator Author

@dakejahl @dagar Is this something that might be useful for PX4 too?

@dakejahl
Copy link
Contributor

I think it would be useful for supporting hot swapping. But to really unlock it we'd need to add a button in QGC

@hendjoshsr71
Copy link
Contributor

Following....

Like you said the bitmask approach is limiting. AP already allows 9 battery instances. (And of course custom code can include far more.)

For the battery reset button on QGC, I do know someone who has done that but I can't promise it will get released soon to the greater community.

@hamishwillee
Copy link
Collaborator Author

The rule for common nowdays is that things need to have one stakeholder implementation (i.e. ArduPilot) and have a firm intent for implementation in another (in this case PX4). What they means is that "someone" has to stand up and say "I will implement this in PX4". I'm not sure if it is worth it if they aren't going to commit to adding to QGC as well.

My guess is that no one is going to stand up on the PX4 side and make that commitment. Mostly because the main audience who will want to hot swap are likely to be thinking about smart batteries anyway. Those that aren't thinking smart batteries probably wouldn't put the effort in to doing the implementation. I'm hoping I'm wrong - the point of this PR was to gauge interest.

Given the discussion I think I'd also be hoping to propose a new command that makes this more generic. That's even harder because it would require both PX4 and ArduPilot to adopt. If any interest then we can discuss - something like:

      <entry value="xxxx" name="MAV_CMD_FUEL_LEVEL" hasLocation="false" isDestination="false">
        <description>Reset capacity for batteries/fuel tanks that accumulate consumed capacity via integration.</description>
        <param index="1" label="battery_slot">Slot number or battery number</param>
        <param index="2" label="percentage" minValue="0" maxValue="100" increment="1">Battery percentage remaining to set.</param>
      </entry>

@hendjoshsr71
Copy link
Contributor

hendjoshsr71 commented Oct 5, 2022

MAV_CMD_FUEL_LEVEL seems fine by me.

If it was adopted I can easily add it to AP along side the current implementation of MAV_CMD_BATTERY_RESET until such time MAV_CMD_BATTERY_RESET could be removed from AP.

I guess there are fewer folks on PX4 using fuel systems? A search in the docs for "fuel" didn't come up with more than 3 hits.

@dakejahl
Copy link
Contributor

dakejahl commented Oct 5, 2022

If it was adopted I can easily add it to AP along side the current implementation of MAV_CMD_BATTERY_RESET until such time MAV_CMD_BATTERY_RESET could be removed from AP.

I could easily add it to QGC as well, I'd be happy to do this for the sake of supporting it. I could probably add it to PX4 as well but don't really want to commit to that since it might be more involved to access the battery module directly and change its state. Idk, I'd have to look to know for sure.

@hamishwillee
Copy link
Collaborator Author

I guess there are fewer folks on PX4 using fuel systems? A search in the docs for "fuel" didn't come up with more than 3 hits.

Yes. It seems to be an "area of interest". I just haven't had anything concrete to add to PX4 docs yet.

I'll ask in the dev call. I know that there are a few messages around fuel systems but really it would be good to have started from "what do we need" rather than "how do we extend our battery messages". Might have had same result, but I think we'd have captured more stuff up front.

@hamishwillee
Copy link
Collaborator Author

We discussed in the dev call. The PX4 position was that this works for ArduPilot so would probably be fine for PX4. However there is no particular need to merge it right now. Since there is no commitment to integrate in PX4 we should not merge at this time.

I'm going to close this. We can reopen and merge when it is needed.

Thanks for the discussion. Should simplify things later on.

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.

None yet

3 participants