Show the limit in motors graph #472
Conversation
|
Thanks, McGiverGim! The logs I have with motor_output_limit less than 100 appear to do the right thing at the higher end of the throttle. At the low end, a log with dynamic and motor_output_limit shows a throttle setpoint of 6.2% (the configured idle percentage), but the motor percentage says 9% (probably from motor_output_limit) while the displayed motor percentage is shown as 0%. It would be good if the motors traces matched the percentage shown in the numbers. In other words, so that the displayed motor range is from 'absolute DShot zero' to 'absolute DShot max'. In other words, motors should:
I'm hoping that throttle setpoint would be shown as a percentage from 0-100 related to stick position, but the motors tab percentage should show the current DShot drive as a percentage of the full DShot range.
|
|
@ctzsnooze I'm not sure if I understand you. In my graphs I see the same value in the right panel, in the graph, or in the table for motors. And is the value that has been written into the blackbox. We will need too some logs with other different motor protocol (oneshot, multishot, proshot) to see what are the max and min values they use, I'm not too sure if they use the same scale than dshot. We don't need a real flight, a simple FC at the desk is enough, only some log with minimum and maximum throttle to see what they send. At this moment all my sdcard are damaged so I can't record them by myself. |
|
I will make some better test log soon, apologies for being slow. The image above shows motor graph indicating 0%, but showing as a number as 9%, while throttle setpoint shows idle percentage (in this case 6.2). It seems to me that the motor graph should show actual DShot idle number in the range 0-2047, with the number shown as the percentage of the 0-2047 range. At idle, throttle setpoint should, I think, indicate the configured idle percentage, so at idle, all traces should overlap if motor_output_limit is 100. |
|
How do you know that the motor graph shows a 0%? I think it is 8%. Can you add other line of something that goes from 0 to any other value to see if it goes under the motor line? I have ignored the minimum value of 48 for motors, maybe the percentages are not accurate for this reason. I will change that and update the code. I suppose: 48 -> 0%, 2047 -> 100%. About showing dshot value, I think is easier to users understand values from 0 to 100% and not from 48 to 2047. And more if it changes from an analog protocol to dshot. But this can be discussed later. It will not be difficult to show if needed. |
|
Ahh! I know, your setpoint is 6% and the motors 8%, and setpoint is over motor. Can you attach your log, please? |
|
It is not an ideal log, because I can't remember if it it has motor output limit enabled... It will do. I will make another that has easier values to test with, from current master. This is not from current master. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
ed0b879 to
25662fd
Compare
|
@ctzsnooze fixed! Now it looks much better, please test the latest code. If it works ok, I think we have pending:
|
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
25662fd to
e0de249
Compare
|
Fixed some offsets... another thing to decide if finally we stay with percentages: now is strange that the setpoint throttle uses one decimal, but the motors have no decimals. Maybe we need to unify both. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
3 times, most recently
from
61c3383 to
c8c0276
Compare
|
Uploaded a new version that:
|
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
c8c0276 to
62901f6
Compare
|
After talking with @ctzsnooze it seems he is happy with the final result of this PR. So ready to be reviewed, modified if necessary and merged. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
62901f6 to
ae5f7db
Compare
js/flightlog_fields_presenter.js
Outdated
| @@ -511,7 +511,7 @@ function FlightLogFieldPresenter() { | |||
| case 'motor[5]': | |||
| case 'motor[6]': | |||
| case 'motor[7]': | |||
| return Math.round(flightLog.rcMotorRawToPct(value)) + " %"; | |||
| return `${Math.round(flightLog.rcMotorRawToPct(value))} %`; | |||
There was a problem hiding this comment.
With a float we get more precision? Using 1‰ should be enough? But didn't look further if that precision can be used in the graphs.
| PWM | Dshot | MotorPct |
|---|---|---|
| 1000 | 0 | -2.40120 |
| 1001 | 48 | 0.00000 |
| 1002 | 50 | 0.10010 |
| 1003 | 52 | 0.20020 |
| 1004 | 54 | 0.30030 |
| 1005 | 56 | 0.40040 |
| 1006 | 58 | 0.50050 |
| 1007 | 60 | 0.60060 |
| 1008 | 62 | 0.70070 |
| 1009 | 64 | 0.80080 |
| 1010 | 66 | 0.90090 |
| 1011 | 68 | 1.00100 |
| 1012 | 70 | 1.10110 |
| 1013 | 72 | 1.20120 |
| 1014 | 74 | 1.30130 |
| 1015 | 76 | 1.40140 |
There was a problem hiding this comment.
Yes, I commented it somewhere to add one decimal to the percentage (more than one is too much).
Another solution is to let the percentage, and add the "real" motor value:
For example: 23 % (46) or the inverse 46 (23 %).
There was a problem hiding this comment.
In true DShot values, we have 1000 steps? So perhaps one decimal point on the percentage?
| @@ -1147,7 +1147,16 @@ FlightLog.prototype.rcCommandRawToThrottle = function(value) { | |||
|
|
|||
| FlightLog.prototype.rcMotorRawToPct = function(value) { | |||
| // Motor displayed as percentage | |||
| return Math.min(Math.max(((value - this.getSysConfig().motorOutput[0]) / (this.getSysConfig().motorOutput[1] - this.getSysConfig().motorOutput[0])) * 100.0, 0.0),100.0); | |||
| let motorPct; | |||
There was a problem hiding this comment.
Why are we changing this - we are sending the motor limits explicitly to be able to use the correct values and not having to make assumptions.
There was a problem hiding this comment.
Which assumptions are you referring to?
There was a problem hiding this comment.
All that are made in this proposal.
There was a problem hiding this comment.
That's not actually helping us get anywhere now is it :-)
There was a problem hiding this comment.
I've been using this build for about 200 or more log analyses now.
In day to day use, focusing on idle and motors, it seems perfect to me.
So if you are proposing a change, then please be constructive and indicate what exactly you think should be changed.
js/graph_config.js
Outdated
| @@ -261,10 +261,11 @@ GraphConfig.load = function(config) { | |||
| try { | |||
| if (fieldName.match(/^motor\[/)) { | |||
| return { | |||
| offset: -(sysConfig.motorOutput[1] + sysConfig.motorOutput[0]) / 2, | |||
| offset: sysConfig.fast_pwm_protocol < FAST_PROTOCOL.indexOf('DSHOT150') ? | |||
There was a problem hiding this comment.
Again, this should use the actual values for the range.
There was a problem hiding this comment.
What actual values should be used?
There was a problem hiding this comment.
The ones for the range - or what others can you think of?
There was a problem hiding this comment.
I'm not intending to be obtuse to annoy.
I really do not know what range you are referring to.
You could mean the lowest or highest value received in the log for all I know.
|
Let's establish some facts to facilitate a discussion on this:
So, my proposal to solve this in a generic way is:
|
|
This is how, with these changes, a log is displayed when there is no dynamic idle, a default 5.5% idle, and no scaling or anything odd. |
|
This is an image showing a quad with dynamic idle active, and the default 5.5% idle, showing the motors occasionally going to zero percent throttle, or, in other words, to a zero DShot drive value. |
|
@mikeller To me this seems exactly how McGyverGim and I, and most people I know, would actually want it displayed. We can see at a glance whether or not dynamic idle is configured. |
|
@ctzsnooze: As I said before, calling something that is based on a range that starts at a point that has got no relation to any physical a 'percentage' is not a good approach. Much rather, the current 'motor output percentage' graph should be fixed so that it displays the output percentage the way all users are used to. But I can see where you want to go with this, and I think the right approach is to add a new graph for 'motor output value (normalised)' - this graph will be 1000 points both for Dshot and analogue protocols (as this is the range inherent to both protocols), and give you what you want - a scaled down version of the motor percentage based on the adjustments like output scaling and dynamic idle. |
|
I can't comment on the best technical implementation approach. However from the user perspective, just because something was done a certain way in the past is not a sufficient reason to continue with it in the future. The original motors graph made sense for the old analog protocols, where there was no absolute zero. We are not changing anything for those users. With digital protocols, and especially with dynamic idle, things are very different. There is an absolute zero value for the motors, and our idle sits above this at some point. The old display method is and was inherently misleading, since it implied that the motor drive was zero when the throttle was zero. This was never true in any protocol, but in the analog days there was nothing we could do about it. Now we have digital control protocols and know what the absolute zero is, we can fix this. It's my view that we need to break from the display of the past so that the user gets 'truthful' information about what the motors are being instructed to do. I don't agree that we should make an entirely different motors display. That would be even more confusing. People would keep asking why one was different from the other. I am very confident that those who view logs will come to very quickly appreciate the virtues of this change. It is so much more logical than the old situation, and so obvious, in retrospect, that people will very quickly appreciate the benefit. So I vote that we should keep the PR as is, other than any technical improvement to the coding, because it does exactly what we should have done long ago. |
|
Mike, I probably look at more log files per day than anyone else on earth. Honestly. I deal nearly every day with people who are trying to interpret our logs. |
|
Is clear that we have different points of view:
I suppose that the decision to show the current 0-100% effective range in the past was done because nobody, specially in the times that we used analog protocols, thought that in a future we will have features to limit/expand the ranges. Now we have different requisites so we need to adapt to them. The latest decision is yours @mikeller so if I need to create a new virtual field for this, I can do it, but I don't think is the best solution. |
|
@McGiverGim: My proposal is to add both in parallel:
|
|
Yes, that is what I wanted to say with "create a virtual field". If I'm not wrong, the first part was accomplished with your changes in the firmware part. It changes the header values that are used by the blackbox to decide the range to shown. But I have not tested it. The second one, is the virtual field to create. But I don't think we must normalize it. Move the dshot/analog values to a normalization of 0-1000 (or 0.0-100.0%) does not give anything here I think. We can't show the real 48-2047 for dshot and the value configured as min-max for analog. I can add both informations if needed (real dshot-analog value and percentage). But as I said before, I don't think is the best solution. But if the decision has been taken I will look into it later. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
87d4977 to
7533fd6
Compare
|
Directly uploaded the source with the decimal. Here is the result: |
|
I will fix the Sonar issues later, so wait until then to review the code. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
2 times, most recently
from
8543f58 to
3272405
Compare
|
I think that ready to be reviewed. |
js/flightlog.js
Outdated
| let motorPct; | ||
| if (this.getSysConfig().fast_pwm_protocol < FAST_PROTOCOL.indexOf('DSHOT150')) { | ||
| // Analog protocol | ||
| const ANALOG_RANGE = this.getSysConfig().maxthrottle - ANALOG_MIN_VALUE; |
There was a problem hiding this comment.
This is inconsistent with the rest - the range should be 2000 - 1000 to indicate the maximum possible range, just like it is for Dshot.
There was a problem hiding this comment.
Actually I was wrong - this should use maxthrottle and minthrottle, and should also use minthrottle to normalise the offset.
There was a problem hiding this comment.
I was unsure too when I implemented it. The objective of this new motor vision is to show the complete "physical" motor range, from motor stop, to motor at its physical max. In dshot is easy, it has a range from 48 to 2047 for any motor, but in analog I was unsure.
If I remember right, in analog protocols we needed to do a "calibration" that restricts the range that can be pased to the values minthrottle to maxthrottle, but I was not sure if minthottle was the motor stop or was the "analog idle" value.
If it is the motor stop, I think you are right and I we need to restrict to it. If it is the "idle" value then I think we need to represent the motor stop too to maintain coherence between both protocols and we need to send lower values too to show the motor stop value, and for this reason I use the 1000 value.
As I say, I was not too sure, it has been so long since I used analog protocols...
There was a problem hiding this comment.
min_throttle is the lowest sent to any motor while flying, and normally would reflect a stable idle at zero stick.
min command is the value to be sent when the motors should be stopped.
In the old motors display we saw 0% when at minthrottle and 100% when at max throttle, both in graph and numbers.
The analog situation around min_throttle is not quite the same as the DShot situation. With DShot, zero is always some consistent point below where the motors actually spin. The relationship between the user's idle point and the zero point is reasonably consistent with DShot, and has some meaning, especially when dynamic idle is present.
For analog protocols, min_command can be set much lower than the spin-up point. It is not always set to 1000. Where it is set is of no consequence once flying, since the motor drive signal cannot go below the configured min_throttle value, and dynamic idle cannot be applied.
To be comparable to DShot in the new display, we would show min_command as the zero point, and then show the motor drive signal as a percentage about this.
If we did this, motors should show zero percent only at idle, or during motor_stop, and then jump to min_throttle on arming, and never go below min_throttle while flying. I can't envisage anything useful coming from this, since there is no configuration that can ever cause motor drive to go below min_throttle while in flight, and it doesn't matter what the offset is down to min_command.
I can envisage a benefit from the existing method, where min_throttle is shown as 0%. In this PR as originally envisaged, only the analog protocol would go to zero consistently while in flight. No DShot protocol would do that, they would all have some DShot offset.
Hence by not having an offset of any kind when an analog protocol is chosen, the graph alone will immediately tell us if the user has an analog ESC connection or a Digital connection.
By my thinking there is no need, and no benefit, of displaying the offset between min_command and min_throttle in any motors display. And I perceive an advantage in NOT doing so.
In the past we only ever showed a simple percentage above min_throttle, and we've never had a complaint about that. I see no virtue in changing from the historical depiction.
For the analog protocols, perhaps showing the actual PWM value that was sent to the motors might be useful, in brackets after the percentage. Whether it is actually useful to display this, I don't know, it's never been a request and to date I haven't envisaged a use for it. But that is the only change that some users might consider an improvement.
js/flightlog_fields_presenter.js
Outdated
| case 'motorRaw[5]': | ||
| case 'motorRaw[6]': | ||
| case 'motorRaw[7]': | ||
| return `${flightLog.rcMotorRawToPctPhysical(value).toFixed(1)} % (${value})`; |
There was a problem hiding this comment.
And % absolute here. If we change the percentage to 2 decimal places we also can drop the value, as this shows an accurate representation.
There was a problem hiding this comment.
I tried to restrict the size to minimum. We have not too much space here, but I will try.
js/flightlog_fields_presenter.js
Outdated
| case 'motor[5]': | ||
| case 'motor[6]': | ||
| case 'motor[7]': | ||
| return `${flightLog.rcMotorRawToPctEffective(value).toFixed(1)} %`; |
There was a problem hiding this comment.
This should still include the raw value for reference and the percentage should be marked as relative to avoid confusion, so probably ${flightLog.rcMotorRawToPctEffective(value).toFixed(1)} % relative (${value}).
There was a problem hiding this comment.
In the old code it has not raw value, but I can add it too without problem. The same concern about the size but I will try.
|
I very strongly disagree with including a 'legacy' motor page, and adding extra terms such as 'absolute' and 'relative' to deal with the problems that arise from including a 'legacy' motor page. Having one single updated motors panel as per the original PR will be less confusing than providing two and with extra terminology and extra numbers in brackets. The virtue of including include a 'legacy' motors panel is to the user a place they can go to see exactly how it would have looked before. So if we did include it, then we should not change it in any way. The proposal seems to be to include it but also change it. That makes no sense to me. There is a lot of value in keeping the log display simple and concise. We should not clutter it with extra values unless they provide a benefit to the user somehow. If we provide two decimal places for the percentage, that is more than enough accuracy, and for digital protocols will convey the exact value that would have been sent. Since this PR originally always showed DShot values in 'absolute' form, the percentage to convert to a number comparable to both the idle setpoint and the values that appear in our motors tab. No other additional number in brackets is required, nor does it add any additional benefit. I strongly suggest we keep this change clean and simple, making no change to how the analog protocols are displayed, and displaying all digital protocols in absolute range with two decimal points for their percentage. No extra numbers in brackets, no 'absolute' or 'relative' suffixes. That's all we need to do. I am confident that if we do include a legacy panel, it will get essentially no use and will likely be removed in a year or so. As such including it, and worrying about the complications that we get from including it, is just a waste of everyones time and energy. |
|
@mikeller could you maybe indicate, clearly, what you think the goal should be here? I've stated my thoughts, but they disagree with yours, and I think McGyverGim is in the challenging position of having to try to code it. We have discussed this privately, and I felt that a clear description of your position would be helpful. My position, as articulated originally in the PR, and modified somewhat following our discussions, is that we should:
If you want anything different from this, please clearly spell out your vision of how motors data should be displayed, ie if you want to add another panel, how would you name it, how the data would be scaled for analog and digital forms, what ancillary values, if any, besides percentages, would be added, etc. I would be really grateful if you could give us some clarity on this. |
|
Yes please, apart from the technical issues that I need to fix, the final desired result is not clear to me. I know you have talked about this, so if we can agree the final desired result it will be great. @mikeller are you ok with the latest message of @ctzsnooze ? Only one motor graph, the one added in this PR, removing the old one? |
|
@McGiverGim: I am ok with the definition that @ctzsnooze gives for the new graph. So my proposal:
N.b. I will propose a fix to show dynamic idle behaviour in the existing graph once betaflight/betaflight#10307 has been merged. |
|
Understood. I will try to do the changes tomorrow. |
|
@mikeller |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
3272405 to
9b70791
Compare
|
@ctzsnooze: I think it makes sense to use the new graph as the default for new users. |
|
Let's see if I have understood all, this is the final result:
|
|
@McGiverGim: We should leave the existing graph (now 'Motors (legacy)') the way it is, including leaving the motor output values shown in braces after the percentage. |
|
The way it is are you saying with the first changes of this PR? In the current master there is no raw value in the motors graph between braces. Will be ok? |
|
@McGiverGim: I thought we had them? But yes, having the raw values in the legacy motor graph will be valuable, so please add them. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
9b70791 to
b603ad7
Compare
Added as requested. |
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
b603ad7 to
4ef24c7
Compare
McGiverGim
force-pushed
the
motor_output_limits_graph
branch
from
4ef24c7 to
eb2bd08
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #467
Is related to this firmware PR too betaflight/betaflight#10106
I think this is what @ctzsnooze wants, but I'm not too sure because I don't have logs with motor limits enabled to see if it works in all cases.
I opened it as draft to be discussed to see if this is what we want to achieve, so ignore the code until discussed.
Everybody feel free to test and comment.