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

Fix IMU sensor refresh rate selection #3642

Merged
merged 3 commits into from Dec 1, 2023

Conversation

haslinghuis
Copy link
Member

Fixes:

image

@haslinghuis haslinghuis added this to the 10.10.0 milestone Nov 24, 2023
@haslinghuis haslinghuis self-assigned this Nov 24, 2023

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Nov 24, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

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

  • tested good with acc/gyro. could not test mag.
    [edit: setting acc or gyro, both change to new value]

@haslinghuis haslinghuis changed the title Fix sensor refresh rate selection Fix IMU sensor refresh rate selection Nov 24, 2023
@ctzsnooze
Copy link
Member

Thanks @haslinghuis for looking into this.

When I test this PR at the current point, changing either gyro or accelerometer will change the other one; they cannot be set to different 'speeds', they are always the same. The displayed 'speed' of one will change when the speed of the other is changed.

Mag appears to be locked to the speed of Gyro or Acc. Adjusting it's speed has no effect, it does nothing. The speed indicated for Mag does not get updated to the speed of Gyro or Acc, even though the gyro/acc speed is what actually is applied.

In contrast, changing either gyro or accelerometer has no effect on Debug or Altitude. Debug and Altitude appear to be entirely independent, they can be set to their own speed.

I am thinking about what the intended behaviour should be?

Is it intended that each trace will be drawn at whatever 'speed' the user selects, ie, that for each trace, it's 'speed' can be set completely independently of the others? In which case, we have separate speed controls for each trace.

Or, alternatively, should each trace be 'locked' so that the speed setting is always the same for all traces - in which case we only need a master speed setting, we don't need individual speed settings.

If we want to have a combination of both behaviours, then we need the ability to 'lock' some traces to the others, and 'unlock' others.

One way could be to have a 'lock'/'unlock' icon next to the speed value. This would be 'locked' by default. Changing the speed of any one of the 'locked' traces would then change all the other 'locked' traces to the new speed. If the user opens the 'lock', however, then that trace becomes 'independent', and can be set to whatever the user wants, and won't be affected by the other ones.

An alternative could be to have a 'master' speed setting at the top. If the master speed is changed, then the speed of all traces is updated to that speed. However, if the speed setting of an individual trace is changed, the result affects only that single trace, not any of the others.

In a practical sense, I typically like all the traces to align and be at the same speed.

So if we had a 'master' speed control, it should reset the screen and reset/sync all vertical grid lines and the 'time' numbers that appear under the traces as well. Currently they get out of sync easily.

Maybe the idea of a 'master speed control' would simplify things a bit?

@haslinghuis
Copy link
Member Author

You need to know that underlying gyro, accel and mag data is filled up using same MSP_RAW_IMU call. That's why a different refresh rate does not make sense here.

// data pulling timers
if (checkboxes[0] || checkboxes[1] || checkboxes[2]) {
GUI.interval_add('IMU_pull', function imu_data_pull() {
MSP.send_message(MSPCodes.MSP_RAW_IMU, false, false, update_imu_graphs);
}, fastest, true);
}

Further improvement could be indeed but are out of the scope of this PR:

  • refresh button to sync graphs
  • master speed control could be used a to reset all rates to the same value - but which value.
  • do not see the need for independent locks

@mituritsyn
Copy link
Contributor

mituritsyn commented Nov 25, 2023

@haslinghuis thanks for fast respond.
Magnetometer refresh rate doesn't work
very minor: if you have only one graph (Gyro/Accel/Mag) running at max refresh rate and then add another one - the speed slows down at the same refresh rate.

@haslinghuis
Copy link
Member Author

@mituritsyn - should be fixed now.

This comment has been minimized.

Copy link

sonarcloud bot commented Nov 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@ctzsnooze
Copy link
Member

My point is that the current user interface suggests that the 'speed' of each trace is independent, because each trace has it's own pop-up. Given that user interface, we should provide each trace with an independent 'speed' from its pop-up.
The user doesn't care about the technicalities of how the data is obtained, they care only about seeing the data at a particular rate that is best for their purpose.
If acc, gyro and mag are all pulled from IMU at the same rate, then whatever is the fastest user-configured rate of those three clearly has to be the rate at which the data for all three are retrieved.
However, the display 'speed' for each trace can still be independent; if say gyro data is at 100Hz, but the user only wants to show Mag at 10hz, then we should be able to do that.
The point I was making about having a 'master' speed control is simply that we can put a drop-down at the top of the sensors panel which can be used to set all values to the same 'speed'. This allows a means for the user to change all channels to the same speed very easily, which is usually what most of us want to do. That's easier than having to change multiple channels one by one.

@ctzsnooze
Copy link
Member

Latest version works 'as intended', in that Acc, Gyro and Mag all share common speed, and cannot be set independently.

I'm a bit confused about why we want different graph speeds for the other channels, but they can be set independently.

Can I ask what the numbers under the vertical grid lines represent? Are they sample counts, or time intervals?

@haslinghuis
Copy link
Member Author

The number is generated using tick interval used in a transition using D3 library.

helpers.xAxis = d3.axisBottom()
.scale(helpers.widthScale)
.ticks(5)
.tickFormat(function (d) {return d;});

@haslinghuis haslinghuis merged commit e18e363 into betaflight:master Dec 1, 2023
9 checks passed
@haslinghuis haslinghuis deleted the fix-sensor-refresh-rate branch December 26, 2023 01:40
chmelevskij pushed a commit to chmelevskij/betaflight-configurator that referenced this pull request Apr 27, 2024
* Fix sensor refresh rate selection

* Fix html

* Refactor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

None yet

6 participants