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

Hide telemetry protocols for CRSF, FPORT and GHST #3726

Merged
merged 4 commits into from
Jan 5, 2024

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Jan 2, 2024

image

image

Inspired after watching https://youtu.be/FMbBfef_R-g

@haslinghuis haslinghuis added this to the 10.10.0 milestone Jan 2, 2024
@haslinghuis haslinghuis self-assigned this Jan 2, 2024

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the hide-radio-telemetry branch 2 times, most recently from e3e83ca to 8081cae Compare January 2, 2024 23:26

This comment has been minimized.

@haslinghuis haslinghuis force-pushed the hide-radio-telemetry branch from 8081cae to 35e9c0f Compare January 2, 2024 23:38

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Jan 3, 2024

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

@nerdCopter
Copy link
Member

i am not sure i understand here. when i flash either a cloud-build or a non cloud-build 4.5 or 4.4, i already do not see these telemetry-protocols in master, or am i mistaken somehow.

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 4, 2024

This PR hides the telemetry protocol header and input if CRSF, FPORT or GHST are selected here

image

@nerdCopter
Copy link
Member

i see now,
image

i thought it was the individual options, but it is the field/header as you explained.

@ctzsnooze
Copy link
Member

ctzsnooze commented Jan 5, 2024

Yes, this has always been confusing.

I would have preferred that when CRSF was selected, the telemetry protocol would show CRSF, and not be editable.

That's the situation; telemetry is an automative part of CRSF; we do not require any extra setting to enable it, it's always there. So a nicer display element would indicate that it was present and active - because it is.

Same for any Radio protocol that automatically enables telemetry when selected.

@KarateBrot
Copy link
Member

KarateBrot commented Jan 5, 2024

We should also make sure to forcefully deselect all selections in that telemetry dropdown before hiding it. (Unless you are already doing that and I can't see it 😃)

@haslinghuis haslinghuis force-pushed the hide-radio-telemetry branch 3 times, most recently from ea07103 to c310695 Compare January 5, 2024 12:18

This comment has been minimized.

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.

  • re-approving for new ....select2('destroy');

@haslinghuis
Copy link
Member Author

Unfortunate this does not work. It works when empty but want to keep the [None] option.
Need to find a way to select2() the [None] option. Another issue when using [None] as place holder we cannot unselect the telemetry protocol when using SBUS for example.

@haslinghuis haslinghuis marked this pull request as draft January 5, 2024 14:23
@nerdCopter
Copy link
Member

after testing, it looked different today as well
CRSF selected:
image
image

SBUS selected:
image

@nerdCopter nerdCopter self-requested a review January 5, 2024 14:40
@haslinghuis haslinghuis force-pushed the hide-radio-telemetry branch from c310695 to 16f28ef Compare January 5, 2024 15:05
@haslinghuis haslinghuis force-pushed the hide-radio-telemetry branch from 16f28ef to ff3234e Compare January 5, 2024 15:06
@haslinghuis haslinghuis marked this pull request as ready for review January 5, 2024 15:09
@haslinghuis
Copy link
Member Author

All sorted.

This comment has been minimized.

@nerdCopter
Copy link
Member

🚀
image
image

@nerdCopter
Copy link
Member

nerdCopter commented Jan 5, 2024

there is a bug. to reproduce:

  1. select IBUS,
  2. select IBUS EXTENDED,
  3. select CRSF,
  4. select IBUS,
  5. select IBUS EXTENDED -- it will set to None.

same for other protocols. they are removed, but not return when protocol is changed.

@haslinghuis haslinghuis marked this pull request as draft January 5, 2024 16:25
@haslinghuis haslinghuis marked this pull request as ready for review January 5, 2024 18:17

This comment has been minimized.

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.

d880010b seems to work proper.

edit: check SonarCloud

@ctzsnooze
Copy link
Member

Could I see a screenshot of how it looks now when the telemetry is 'included', ie when user selects CSRF? Saying 'automatically included', or similar, in the box to the right, kind of works for me, so long as those words don't appear in the drop-down. And I guess if it is 'included' then we don't need the drop-down to be available?

@nerdCopter
Copy link
Member

  • screenshot in description @ctzsnooze .
  • Currently says "This Radio Protocol has Telemetry included", but i think "Automatically Included" is fair as well.

@haslinghuis
Copy link
Member Author

haslinghuis commented Jan 5, 2024

Drop down is not available when telemetry is included. Updated screen shot.

Copy link

sonarcloud bot commented Jan 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

github-actions bot commented Jan 5, 2024

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!

Copy link
Member

@ctzsnooze ctzsnooze left a comment

Choose a reason for hiding this comment

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

Thanks! This looks really good. Very much like to see "automatically included", much nicer than before.

@haslinghuis haslinghuis merged commit 03b0233 into betaflight:master Jan 5, 2024
8 checks passed
@haslinghuis haslinghuis deleted the hide-radio-telemetry branch January 5, 2024 21:32
chmelevskij pushed a commit to chmelevskij/betaflight-configurator that referenced this pull request Apr 27, 2024
* Hide telemetry protocols for CRSF, FPORT and GHST

* Add placeholder

* Update message

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

Successfully merging this pull request may close these issues.

5 participants