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

Added battery cell count based automatic PID profile switching. #7516

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
3 participants
@mikeller
Copy link
Member

mikeller commented Feb 3, 2019

This implements switching of PID profiles based on the cell count of the battery that is connected, as proposed in #7481.

It introduces the PID profile parameter auto_profile_cell_count supporting these values:
0: If this profile is selected, never automatically switch to a different profile, irrespective of the detected cell count (default);
-1: If this profile is selected, switch to the next profile that has a auto_profile_cell_count matching the detected cell count. If there is none, switch to the next profile having 0 for auto_profile_cell_count;
1 <= x <= MAX_AUTO_DETECT_CELL_COUNT: If this profile is selected, use it if the detected cell count is x. Otherwise, switch to the next profile that has a auto_profile_cell_count matching the detected cell count. If there is none, switch to the next profile having 0 for auto_profile_cell_count.

Fixes #7481.

@mikeller mikeller added this to the 4.0 milestone Feb 3, 2019

@etracer65

This comment has been minimized.

Copy link
Member

etracer65 commented Feb 3, 2019

-1: If this profile is selected, switch to the next profile that has a auto_profile_cell_count matching the detected cell count. If there is none, switch to the next profile having 0 for auto_profile_cell_count;

I'm not sure I understand the use-case for this. Seems like it would prevent any profile set to -1 from ever being used.

1 <= x <= MAX_AUTO_DETECT_CELL_COUNT: If this profile is selected, use it if the detected cell count is x. Otherwise, switch to the next profile that has a auto_profile_cell_count matching the detected cell count. If there is none, switch to the next profile having 0 for auto_profile_cell_count.

For this behavior to be understandable it would seem like we need to enforce uniqueness for auto_profile_cell_count otherwise the results could seem somewhat random. If I had 2 4-cell profiles I'd never know which one was selected.

I'm wondering if we're looking at this wrong and should turn it on it's head. Rather than being a value associated with a profile perhaps we just need discrete settings like a battery_profile command (or something like that) that sets the mapping from battery cells to profile. Let the users define:

battery_profile <cell count> <profile_number>

For example the user may want to associate 5S and 6S to a specific profile. Assigning it as a pidProfile value wouldn't allow this.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 4, 2019

I'm not sure I understand the use-case for this. Seems like it would prevent any profile set to -1 from ever being used.

A -1 profile will be used if there is no other option that matches, i.e. it is a 'generic' profile.

Combining -1 profiles and 0 in the same setup probably does not make much sense, and this is something that should be pointed out in the documentation.

For this behavior to be understandable it would seem like we need to enforce uniqueness for auto_profile_cell_count otherwise the results could seem somewhat random. If I had 2 4-cell profiles I'd never know which one was selected.

Not sure of that. The behaviour is always deterministic based on the selected profile - the next (in round robin) matching profile is selected. I can see a use case where say a user wants to fly 4S, and 6S either for acro or for racing, and sets 2 profiles to 6. If the 4 profile or the first 6 profile after it are selected when powering up with a 6S battery, the first 6 profile is chosen. If the second 6 profile is selected, this one stays. When powering up with a 4S battery, the 4S profile is always chosen.

I am well aware that how this is done is on the 'versatile' side of the 'versatile' - 'simple' scale, and that dumbing it down to some extent might be better. Just not sure where the sweet spot lies.

I'm wondering if we're looking at this wrong and should turn it on it's head. Rather than being a value associated with a profile perhaps we just need discrete settings like a battery_profile command (or something like that) that sets the mapping from battery cells to profile. Let the users define:

battery_profile <profile_number>
For example the user may want to associate 5S and 6S to a specific profile. Assigning it as a pidProfile value wouldn't allow this.

I am not in favour of this - I think we need to try to restrict how many custom commands are added and how much custom syntax is introduced in CLI, if something equivalent can be achieved by adding parameters that can use getters / setters - the cost for maintaining custom code in CLI is just too high, and it is prone to having bugs.

Say a user wants to use 4S, 5S, and 6S, and use the same profile(s) for 5S and 6S, they can get the desired behaviour by having the 4S profile as 4, and the others as 0.

@mikeller mikeller force-pushed the mikeller:add_automatic_pid_profile_switching branch from 687cbe7 to 299d96f Feb 4, 2019

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 5, 2019

@etracer65, all: What's your verdict on this now? Should we go for it, or should we try to simplify this more (if so, how)?

@etracer65

This comment has been minimized.

Copy link
Member

etracer65 commented Feb 5, 2019

@mikeller I still think the logic to select the desired profile is too cryptic and implicit rather than explicit. Users are going to approach this from the perspective of what they want to happen when they connect a 5S battery - the explicit case. As it is now they have to figure out what will happen as a result of connecting the 5S battery.

Some points I think are important:

  1. There should only be one profile for a given cell count. I can't think of any use case other than confusion if we allowed say 2 different profiles to be set for 4S.
  2. It's a reasonable use case that I may want to have multiple cell counts use the same profile. For example I might have a profile that I use for both 5S and 6S.
  3. There are more cell count options than profiles so driving it from a profile setting becomes a problem. Say for example I want 3S = profile 1, 4S = profile 2, 5S/6S = profile 3.
  4. How to handle a cell count not mapped to a profile? I think the logic should be just keep the currently selected profile. This matches the current usage before auto-detection.

I still think some kind of battery cell -> profile mapping setup is the best and most flexible. I understand the aversion to more custom CLI commands though. Not as clean but an alternative would be to have explicit parameters for each cell count like profile_select_1s, profile_select_2s, etc. The default would be NONE.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 5, 2019

@etracer65:

Users are going to approach this from the perspective of what they want to happen when they connect a 5S battery - the explicit case. As it is now they have to figure out what will happen as a result of connecting the 5S battery.

Point taken.

Let me play devil's advocate here:

There should only be one profile for a given cell count. I can't think of any use case other than confusion if we allowed say 2 different profiles to be set for 4S.

So there won't be any users that will want to use batteries with the same cell count for say racing and freestyle? I find this hard to believe.

It's a reasonable use case that I may want to have multiple cell counts use the same profile. For example I might have a profile that I use for both 5S and 6S.

auto_profile_cell_count == -1 does exactly that.

There are more cell count options than profiles so driving it from a profile setting becomes a problem. Say for example I want 3S = profile 1, 4S = profile 2, 5S/6S = profile 3.

profile 0
auto_profile_cell_count == 3

profile 1
auto_profile_cell_count == 4

profile 2
auto_profile_cell_count == -1

How to handle a cell count not mapped to a profile? I think the logic should be just keep the currently selected profile. This matches the current usage before auto-detection.

This can be easily done, see the configuration in the example above. And the default of having auto_profile_cell_count == 0 will do this for each profile individually.

Not as clean but an alternative would be to have explicit parameters for each cell count like profile_select_1s, profile_select_2s, etc. The default would be NONE.

Ok. How do you propose that we solve the problem of allowing users to specify more than one profile that are viable as presets for the same cell count? Should we define an array [possible cell count][number of profiles - 1] to allow users to define more than one possible profile for any given cell count?

@etracer65

This comment has been minimized.

Copy link
Member

etracer65 commented Feb 5, 2019

PID Profiles are (supposed) to be about things that are affected by the physical characteristics of the craft. PID settings are the primary things that would change based on the battery cell count. When considering racing vs. freestyle we should be talking more about rate profiles, not PID profiles. So I still don't really see a use case for two different profiles for the same cell count. Even if we assume it's valid, then the current design with the cell count as part of the profile still leads to ambiguity. Assume two profiles set as 4S - Which one should be used? How do I know which is selected? How do I switch to the other if the wrong one is selected? I'm not sure how we get there without developing an overly complicated settings structure that I don't feel is warranted.

I think drawing the line at one profile per cell count is really the intent of the need. The common scenario is that a quad that's tuned perfectly on 4S my have severe oscillation and need lower P/D values for 6S.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 5, 2019

@etracer65:

PID Profiles are (supposed) to be about things that are affected by the physical characteristics of the craft. PID settings are the primary things that would change based on the battery cell count. When considering racing vs. freestyle we should be talking more about rate profiles, not PID profiles.

I think there is a basic difference in what we see in this: You seem to think of this as a 'convenience feature'. I see this as a safety enhancement (i.e. stop users from using PID profiles with batteries that will burn their motors / ESCs), since this was proposed in the context of per-PID-profile motor output limits (#7481). In this context, I want to make it possible for users to test with 2 different PID profiles for the same battery size, and still not expose themselves to accidentally starting up with the wrong profile selected and burn their motors / ESCs. With your proposal this is not possible, so users looking for a safety feature and wanting to use PID profiles for testing will not be able to use this feature, exposing themselves to unneeded risk.

Even if we assume it's valid, then the current design with the cell count as part of the profile still leads to ambiguity. Assume two profiles set as 4S - Which one should be used?

To me, not accidentally using the 2S profile and destroying the craft is more important in this case than getting the absolutely right profile.

Let's be clear here - users have to consciously create such an ambiguos configuration, and my assumption is that if they do they have a good reason to do so.
But if you feel so strongly about not allowing users to consciously create a configuration that has multiple 'right' profiles for a given battery size (and using the PID profile indicator in OSD to verify that the right one is used), we can always add a configuration validation that excludes this - I just don't think allowing users to create these configurations is a safety risk, and should be prevented.

Which one should be used?

See my initial comment for an explanation.

How do I know which is selected?

Confirmation beeps, OSD indicator, blackbox logs.

How do I switch to the other if the wrong one is selected?

PID profile switching stick commands.

I think drawing the line at one profile per cell count is really the intent of the need. The common scenario is that a quad that's tuned perfectly on 4S my have severe oscillation and need lower P/D values for 6S.

Can you outline how, in this scenario, if a user wants to A/B test a new PID configuration for 6S, how can they use your system to avoid accidentally starting up on the 4S profile and burning their craft, while still being able to select their new 6S PID profile in the field, and not have it reset to their old 6S profile every time they plug in a battery?

@etracer65

This comment has been minimized.

Copy link
Member

etracer65 commented Feb 6, 2019

I would contend that if somebody was really doing A/B testing then they should be manually selecting profiles anyway. Relying on the auto detection picking the correct one of multiples that could be matched seems like a poor way to test.

To simplify my suggestions:

  1. User should be able to specify which profile is selected for a given cell count.
  2. The same profile can be chosen for multiple cell counts. Allows using the same profile for 5S and 6S as an example.
  3. If the detected cell count does not have a profile assigned then leave previous profile selection unchanged.
  4. User can manually change the profile (with stick commands/OSD) after the initial auto-detect.

To me this covers everything.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 6, 2019

@etracer65:

These suggestions seem to be sound to me. The current code implements all of these, in a way that does not use a lot of flash space.

I would like to add, for safety reasons:

  1. A user can specify more than one profile that are safe to be used for a given cell count.

This is a safety feature that is implemented in my solution, and can be added to your proposal.

Also, keep in mind that what we are discussing now is the 'CLI only' implementation. In my opinion, this should users give full configurability, but does not need to be optimised for usability - once we add this to MSP, the UI can probably be made look pretty much the same no matter what implementation is chosen.

@etracer65

This comment has been minimized.

Copy link
Member

etracer65 commented Feb 7, 2019

I would like to add, for safety reasons:

  1. A user can specify more than one profile that are safe to be used for a given cell count.

This is a safety feature that is implemented in my solution, and can be added to your proposal.

To me this is the main thing that leads to confusion as it causes the ambiguous behavior. Ambiguity is not desirable from a safety perspective. I want to know exactly which profile is going to be selected for a given cell count. If there are multiple possible then it leads to potential risk.

I don't have any problems with the overall implementation - just that I think the UX for configuring the behavior is overly complex and confusing. So if you want to proceed with this then I'm okay with it and we can revisit based on feedback.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 7, 2019

A user can specify more than one profile that are safe to be used for a given cell count.
This is a safety feature that is implemented in my solution, and can be added to your proposal.

To me this is the main thing that leads to confusion as it causes the ambiguous behavior. Ambiguity is not desirable from a safety perspective. I want to know exactly which profile is going to be selected for a given cell count. If there are multiple possible then it leads to potential risk.

Let's agree to disagree there. :-)
To me, not using auto select because it is not flexible enough to allow the configuration that the user wants is an even bigger safety risk than the user willingly setting up multiple profiles, one of them not working, for the same cell count - most users will just never make use of this facility.

I don't have any problems with the overall implementation - just that I think the UX for configuring the behavior is overly complex and confusing. So if you want to proceed with this then I'm okay with it and we can revisit based on feedback.

'Good UX' was definitely not a major consideration when implementing this - this is the simplest possible and sufficient way to support the functionality. If it takes, we should look into adding the good UX through the configurator. If we find users are asking for being able to configure multiple cell counts for the same profile we should pivot it.

@mikeller mikeller merged commit 59ea4be into betaflight:master Feb 7, 2019

@mikeller mikeller deleted the mikeller:add_automatic_pid_profile_switching branch Feb 7, 2019

@dabit20

This comment has been minimized.

Copy link

dabit20 commented Feb 15, 2019

Question: is this only valid for PID profiles, or also for rate profiles?

My use case: I am slowly moving toward 6S. Quad is a basic 4S setup with 2450Kv motors. Saggy 4S batteries are transformed into 5S where the 20% higher capacity and lower average current for the same thrust prevents early low battery warnings. Broken/depleted batteries get replaced with 5S batteries. And once the motors and non-6S-capable electronics are broken, these will be replaced with 6S parts and the batteries gradually upgraded from 5S to 6S.

To make 4S and 5S feel the same scaled PIDs are required, but also different values for throttle boost, throttle_limit_percent/throttle_limit_type and possibly thr_mid/thr_expo. Quite a few of these are in the rate profile, not in the PID profile. Currently I switch manually. Automatic switching would be great.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 16, 2019

@dabit20: This works for PID profiles only as it is. Rate profile used to be coupled to PID profiles at some point, but then they were uncoupled because most users were struggling to understand how the coupling worked.
There have been requests to re-introduce an optional coupling between rate profiles and PID profiles, so maybe this could be solved this way?

@dabit20

This comment has been minimized.

Copy link

dabit20 commented Feb 17, 2019

The main thing is that many parameters are not strictly PID or rate so these profiles are not uncoupled anyway. With more agressive props of higher cell count feedforward has more impact, for example.
Re-introducing a coupling between these profiles makes sense IMHO. Therefore it makes sense to (re)introduce a coupling between them. I know that I am changing them in pairs.
Which takes around 10 seconds using the OSD, so all the automatic stuff is not that big of a deal anyway. It would be nice though if the profiles had an associated name instead of only a number.

But it also makes sense to introduce auto_profile_cell_count for every other type of profile. It makes configuration more generic; 'feature A works only with X, feature B works only with Y' limitations should be avoided unless there is a good reason for them, IMHO.

@mikeller

This comment has been minimized.

Copy link
Member Author

mikeller commented Feb 17, 2019

@dabit20: Agreed that the values in PID and rate profiles are coupled in how they are used. The coupling I was referring to was an actual coupling: Every PID profile hat its own 3 rate profiles. This confused users as they set up their preferred rates in one PID profile, and then they switched PID profiles, and they got default rates again. If we are coupling them again I think we need to do better than this.

More broadly, the arrangement of parameters into PID profiles and rate profiles as it is now is due for a review / rearrangement anyway, and we should look if they can be arranged in a way that covers the pilots' needs in a better way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.