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

Update OSD throttle options #8806

Merged
merged 6 commits into from Apr 20, 2023
Merged

Update OSD throttle options #8806

merged 6 commits into from Apr 20, 2023

Conversation

MrD-RC
Copy link
Collaborator

@MrD-RC MrD-RC commented Feb 17, 2023

Fixes #8801

Pre-INAV 3.0 there were two working throttle display elements. A PR in 2.7 (#6510) made a change that meant both OSD throttle elements showed the same data. This is pretty pointless. The previous method allowed the display of the throttle in a non-scaled view, which made selecting navigational throttles easier. This PR allows the pilot to see scaled throttle, unscaled, or both. Both throttles work for manual and auto throttle.

Configurator iNavFlight/inav-configurator#1725

Pre-INAV 3.0 there were two working throttle display elements. A PR in 2.7 made a change that meant both OSD throttle elements showed the same data. This is pretty pointless. The previous method allowed the display of the throttle in a non-scaled view, which made choosing navigational throttles easier. This PR allows the pilot to see scaled throttle, unscaled, or both. Both throttles work for manual and auto throttle.
int16_t thr = constrain(rcCommand[THROTTLE], PWM_RANGE_MIN, PWM_RANGE_MAX);

if (useScaled) {
thr = (thr - getThrottleIdleValue()) * 100 / (motorConfig()->maxthrottle - getThrottleIdleValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

getThrottleIdleValue should be assigned to a const

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. No point in multiple calls.

if (useScaled) {
thr = (thr - getThrottleIdleValue()) * 100 / (motorConfig()->maxthrottle - getThrottleIdleValue());
} else {
thr = (thr - 100) / 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

1000 surely ?

Copy link
Contributor

Choose a reason for hiding this comment

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

thr = (thr - PWM_RANGE_MIN) * 100 / (PWM_RANGE_MAX - PWM_RANGE_MIN);

Copy link
Collaborator Author

@MrD-RC MrD-RC Feb 17, 2023

Choose a reason for hiding this comment

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

@breadoven Yes, should be -1000, typo.

@tiriad Actually, this would be better, as it is based on PWM_RANGE_MIN and PWM_RANGE_MAX.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I only suggested it to follow the style of other variables and the code prior to the modification assuming it was correct.

thr = (constrain(rcCommand[THROTTLE], PWM_RANGE_MIN, PWM_RANGE_MAX) - PWM_RANGE_MIN) * 100 / (PWM_RANGE_MAX - PWM_RANGE_MIN);

@tiriad
Copy link
Contributor

tiriad commented Feb 18, 2023

what do you think if you add in addition a field with the raw value rcCommand[THROTTLE] for the purpose of tunning?

@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Feb 18, 2023

That is effectively what the unscaled throttle percentage is. I think showing the PWM value would be a step backwards. Plus, that could probably be done now using logic conditions and a Gvar.

@tiriad
Copy link
Contributor

tiriad commented Feb 18, 2023

I have already managed to obtain the PWM value. A curious case here is that this value is already a % in the PWM range.

case LOGIC_CONDITION_OPERAND_FLIGHT_TROTTLE_POS: // %
return (constrain(rcCommand[THROTTLE], PWM_RANGE_MIN, PWM_RANGE_MAX) - PWM_RANGE_MIN) * 100 / (PWM_RANGE_MAX - PWM_RANGE_MIN);
break;

@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Feb 18, 2023

It is also not scaled.

@DzikuVx DzikuVx changed the base branch from master to release_6.1.0 March 14, 2023 20:29
@MrD-RC MrD-RC modified the milestones: 6.1, 7.0 Apr 18, 2023
@MrD-RC
Copy link
Collaborator Author

MrD-RC commented Apr 18, 2023

I've got this to a stage where it's as good as it can be without the throttle getting a complete overhaul. The problem is the scaling. This should really only happen before output (to the motors or the OSD if a scaled output is requested). The problem is it happens in places like navigation. This is not just a mess for the OSD. But it means that other areas work differently too. For example, outputting 'rcCommand[THROTTLE]' from logic_condition.c will give a different result to outputting 'rcCommand[THROTTLE]' from mixer.c. mixer.c is where the throttle output is calculated for the OSD. At that point, it has already been scaled.

image
In this image. The throttle on the stick is central (1508us). The outputs in configurator and the 45% throttle on the OSD should be the same value.

For this PR, I've had to compromise. The scaled throttle OSD element is using the same code as before. I don't believe this is necessarily correct, as it is different to the value shown in configurator for the motor output. The unscaled output is correct. But due to rcCommand[THROTTLE] already being scaled at this point, the auto-throttle will have to show the scaled value currently. I tried reversing the scale. But the result was not correct. I think it would be better to remove the scaling, until output. Only then will we get a properly consistent and clean throttle setup.

Either way. It's better than what's currently in INAV. As both throttle OSD elements are identical. The only thing I'm not sure about is the *. This was to differentiate between the two types of throttle. But I think it could be better. Maybe 2 throttle symbols instead of the *. But I'm open to suggestions. Ideally ones that don't need a new font character.

@MrD-RC MrD-RC modified the milestones: 7.0, 6.1 Apr 18, 2023
Rather than having a `*` prefix to indicate the PWM throttle. I'm now using the scale symbol (also used in the map) to prefix the scaled throttle. This makes a lot more sense.
@MrD-RC MrD-RC merged commit 2d7a057 into release_6.1.0 Apr 20, 2023
13 checks passed
@MrD-RC MrD-RC deleted the MrD_OSD-Throttle-Options branch April 20, 2023 21:07
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.

High throttle in automatic modes
3 participants