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

feat(color,cpn) - Updates to the model telemetry page. #3401

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

philmoz
Copy link
Collaborator

@philmoz philmoz commented Mar 25, 2023

This PR take the changes from PR #3150 and adds some additional fixes and cleanup.
I opted to create a new PR instead of trying to work in a fork of a fork :(

I also changed the title for the new option to 'Show instance ID' instead of 'Show sensor ID'.

Screen Shot 2023-03-25 at 4 57 32 pm

Changes:

  • Add 'Show Instance ID' button to telemetry page to show/hide sensor instance number (default is hidden).
  • The 'showInstanceIds' property is saved to the model file.
  • Remove the 'Name', 'Value' and 'ID' labels row from the page.
  • Fix issues with page refresh when all sensors deleted.
  • Select closest sensor when a single sensor is deleted.
  • On change, only refresh the sensor list instead of the whole page.
  • Update Companion to save/load 'showInstanceIds' in model YAML file.
  • Add 'Show Instance IDs" checkbox to telemetry edit page in Companion.

I changed the look for showing the instance ID a bit from PR 3150, to avoid having to change the line height for each row.
Screen Shot 2023-03-25 at 4 38 42 pm

All GPS strings should fit without having to adjust font size on portrait layout:
Screen Shot 2023-03-25 at 4 38 15 pm

Phil Mitchell added 2 commits March 27, 2023 09:07
…nstance number (default is hidden).

Fix issues with page refresh when all sensors deleted.
Select closest sensor when a single sensor is deleted.
On change, only refresh the sensor list instead of the whole page.
The 'showInstanceIds' property is saved to the model file.
Update Companion to save/load 'showInstanceIds' in model YAML file. Add checkbox to telemetry edit page.
@ParkerEde
Copy link
Contributor

@philmoz
nice job. I really like that already.
Unfortunately, there are still two small problems here.

  1. when the sensor discovery is started and the list is filled with found sensors, the focus jumps to the first sensor line (ID0). This is irritating, because you can stop the sensor search only when you scroll to the stop button again. If there are already sensors in the list, the focus also jumps to ID0 and you don't see if the detection is done or not. Here I think the focus should be kept on the Start/Stop sensor search button. Then both cases would be cleanly operable and recognizable.
  1. When I edit a sensor, e.g. change the unit or the precision (combo box), the focus always jumps back up to the "Type" line.

@ParkerEde
Copy link
Contributor

One more small thing to discuss. For a Black/white radio, the first sensor gets ID1. With a color radio the first sensor gets ID0. I would find it good if this would be handled uniformly.

Start sensor ID index at 1 to match B&W radios.
Keep focus on changed parameter when editing sensor, don't rebuild entire parameter list on change.
@philmoz
Copy link
Collaborator Author

philmoz commented Mar 27, 2023

I've made the changes suggested by @ParkerEde

  • The 'Stop' button has focus when searching for sensors.
  • The ID values start at 1 instead of 0.
  • The focus should not jump when editing a sensor.

@elecpower
Copy link
Collaborator

@philmoz can you also look a PR #2698 and the associated Issues to see they are covered off too and then we can close 2698 as superseded by this one.

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 28, 2023

@philmoz can you also look a PR #2698 and the associated Issues to see they are covered off too and then we can close 2698 as superseded by this one.

@elecpower It appears the issues in 2698 are still present.

The B&W radios and the telemetry simulator in Companion display the instance ID +1 while color radios uses instance ID.
Which is correct?

@ParkerEde
Copy link
Contributor

Companion adds +1 to the instance ID. Here a further discussion about it.
#2678 (comment)

@ParkerEde
Copy link
Contributor

I've made the changes suggested by @ParkerEde

  • The 'Stop' button has focus when searching for sensors.
  • The ID values start at 1 instead of 0.
  • The focus should not jump when editing a sensor.

Yes, perfect. I've tested it. Good job. Thx

@pfeerick pfeerick added color Related generally to color LCD radios UX-UI Related to user experience (UX) or user interface (UI) behaviour translation labels Mar 28, 2023
@philmoz
Copy link
Collaborator Author

philmoz commented Mar 28, 2023

I've remove the +1 when displaying the instance ID on B&W radios, telemetry settings in Companion and the telemetry simulator.

I'm not entirely sure why this was done in the first place - hopefully it doesn't break anything.

@pfeerick
Copy link
Member

pfeerick commented Mar 28, 2023

I have no idea either (or why it was +1'd other than apparently for legacy at some point)

@3djc What's the history with the instance IDs... what number should be shown?

@ParkerEde
Copy link
Contributor

Here about the history:
#2678 (comment)

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 28, 2023

Here about the history:
#2678 (comment)

I've read through all that; but I can't see any explanation as to why it was done.

@pfeerick
Copy link
Member

pfeerick commented Mar 28, 2023

Indeed... that's only the what... not the why... i.e. it looks like it originates in OTX 2.3.1, but is that because they were all shifted by one inadvertently in an earlier version? Were they always displayed wrong, and that was just keeping the behaviour the same? etc. But, thank you for the cross-reference ;)

@ParkerEde
Copy link
Contributor

With the latest commit, the instaceIDs in Companion are the same as in the radio/simulator. But now the telemetry simulator does not work anymore. Tested with Horus X10 Express profil

@elecpower
Copy link
Collaborator

Also need to consider are there any data conversion considerations or do we force sensor rediscover and recreate all manually configured sensors after this PR is implemented. That is why I asked the question in the referenced PR and Issues.

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 28, 2023

With the latest commit, the instaceIDs in Companion are the same as in the radio/simulator. But now the telemetry simulator does not work anymore. Tested with Horus X10 Express profil

The telemetry simulator is working for me on TX16S and QX7.
Do the instance ID's match between the radio simulator and the telemetry simulator?

@ParkerEde
Copy link
Contributor

The telemetry simulator is working for me on TX16S and QX7.
Do the instance ID's match between the radio simulator and the telemetry simulator?

Yes. With the version before everthing works fine with the telemetrie-simulator. Try to restart Radiosimulator with this button:
Screen01

@ParkerEde
Copy link
Contributor

directly after starting the simulator, the instance IDs are not read correctly from the model preset and entered in the simulator. If I do this manually, it is okay, as @philmoz also wrote. But if you press the "Reload all Radio Data" button, all sensors are read from the model preset, but then still entered with the InstanceID+1.

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 28, 2023

directly after starting the simulator, the instance IDs are not read correctly from the model preset and entered in the simulator. If I do this manually, it is okay, as @philmoz also wrote. But if you press the "Reload all Radio Data" button, all sensors are read from the model preset, but then still entered with the InstanceID+1.

Should be fixed now.

@ParkerEde
Copy link
Contributor

the reload button now works correctly again. Thx
Should we also take the opportunity to fix the fact that the InstanceID are not read in correctly immediately after the simulator start?
Or rather in a separate ticket/PR like #3047?

@philmoz
Copy link
Collaborator Author

philmoz commented Mar 29, 2023

Should we also take the opportunity to fix the fact that the InstanceID are not read in correctly immediately after the simulator start?

Probably not - it looks messy/complex to change.
The telemetry simulator is initialised when the main simulator window is opened.
At that point in time there is no model loaded in the radio simulator so there is no data to use to set up the telemetry simulator sensors.

@ParkerEde
Copy link
Contributor

tested with QX7 Access Sim.
After reloading, the correct IDs are now displayed in the telemetry simulator.

@ParkerEde
Copy link
Contributor

@pfeerick I think this PR is ready to go to main. Checked this on Horus X10 Express and QX7ACCESS and also in Companion.

@pfeerick pfeerick added this to the 2.9 milestone Apr 9, 2023
@Eldenroot
Copy link
Contributor

Tested, TX16s MKII - should be fine, good improvement. Thank you

@pfeerick
Copy link
Member

Thanks for the checks guys, yes, this also looks good for both EL18 and TX16S.

@pfeerick pfeerick merged commit 6985d3f into EdgeTX:main Apr 17, 2023
ulfhedlund added a commit to ulfhedlund/edgetx that referenced this pull request Apr 18, 2023
ulfhedlund added a commit to ulfhedlund/edgetx that referenced this pull request Apr 18, 2023
@philmoz philmoz deleted the telemetry-update branch October 18, 2023 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color Related generally to color LCD radios translation UX-UI Related to user experience (UX) or user interface (UI) behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants