Enhanced sat list for M8N/M9N #2603

TonyBlit · 2021-09-09T20:15:01Z

Modified the sat list in order to show extra info from the following PR: betaflight/betaflight#10921

New layout:

New sat list in action:

haslinghuis

Thanks for contributing.
Please remove commented out code.
Some remarks I made are valid in different places.
Hint: look at the Sonar code smells.
We should also test if this change is backwards compatible.

haslinghuis · 2021-09-10T20:02:49Z

src/tabs/gps.html

-                                <td style="width: 20%;" i18n="gpsSignalSatId"></td>
-                                <td style="width: 15%;" i18n="gpsSignalQty"></td>
-                                <td style="width: 65%;" i18n="gpsSignalStr"></td>
+                                <!--<td style="width: 10%;" i18n="gpsSignalGnssId"></td>-->


Should this be removed?

Actually, I forgot to ask about this. How does the localization system work? do I have to add gpsSignalGnssId with the "Gnss Id" string to the messages.json in all languages? In this case it's easy, but what about other strings that might need to be translated?

Not, only English. We have translators that will do the rest of languages

haslinghuis · 2021-09-10T20:05:45Z

src/js/tabs/gps.js

+                        let quality = '';
+                        switch (FC.GPS_DATA.quality[i] & 0x7) {
+                            case 0:
+                                quality = quality + 'no signal | ';


Suggested change

quality = quality + 'no signal | ';

quality += 'no signal | ';

I have reworked those ugly switch cases, so this is no longer an issue :-)

haslinghuis

Please also fix the sonar and squash your commits when done. Looks good.

haslinghuis · 2021-09-13T22:37:24Z

src/tabs/gps.html

+                                <td>0</td>
                            </tr>
-                        </table>
+                            </table>


👾 was on the right position 😆

McGiverGim · 2021-09-14T06:35:04Z

Please also fix the sonar and squash your commits when done. Looks good.

Only as a help for @TonyBlit , not all the Sonar issues need to be fixed. Sometimes they have no sense. In this case, the literal that has been duplicated I think it does not need to be fixed, it is part of an array and will be strange to define this outside and not the other elements.
The other two issues can be fixed.

TonyBlit · 2021-09-15T06:19:03Z

@McGiverGim yeah, the first one it doesn't make sense, indeed. The other ones made me think I forgot to localize the messages, so I've fixed everything and squashed, hope all is good now.

McGiverGim

To me is good enough... good work! :)
As suggestion, in the Configurator PRs is good to attach a screen capture. It helps with the review.

haslinghuis

LGTM

McGiverGim · 2021-09-17T07:09:11Z

@TonyBlit I've seen this in the firmware PR:

gps_ublox_use_galileo will no longer disable QZSS as it's suggested on the M8N manual

Can you modify this description:

betaflight-configurator/locales/en/messages.json

Lines 1465 to 1468 in dfd8a92

    
           "configurationGPSGalileoHelp": { 
        
               "message": "When enabled, this removes the QZSS system (Japanese) and replaces it for the Galileo system (European).", 
        
               "description": "Help text for the option to use Galileo in the GPS configuration" 
        
           },

I think something like:

It enables the Galileo system (European). Until Betaflight 4.2 this implied to remove the QZSS system (Japanese) too.

Sorry, I'm not English native, I'm sure you will find something better :)

TonyBlit · 2021-09-18T06:23:47Z

@McGiverGim Well spotted! I was not even conscious of this message.

I've been tempted to mention ublox_use_galileo is only for M8N, but given M9N has galileo already enabled, maybe it's not worth complicating the sentence until M9N is widely adopted.

sonarqubecloud · 2021-09-18T08:15:26Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
49.8% Duplication

limonspb

@TonyBlit please squash in 1 commit and i think its ready to go.

blckmn · 2021-12-03T23:17:20Z

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 -> PASS

src/js/tabs/gps.js

sonarqubecloud · 2021-12-20T07:06:32Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
50.2% Duplication

asizon

Really good features, my requested changes are not to important but if you can take a look would be nice

asizon · 2021-12-20T08:08:06Z

src/tabs/gps.html

-                                <td style="width: 20%;" i18n="gpsSignalSatId"></td>
-                                <td style="width: 15%;" i18n="gpsSignalQty"></td>
-                                <td style="width: 65%;" i18n="gpsSignalStr"></td>
+                                <td style="width: 12%;" i18n="gpsSignalGnssId"></td>


The use of styles into html part is old, now we try to move all styles in to css file.

I wouldn't block this though. Both inline styles and styles files are redundant if we just move to vue/component based architecture, so one way or the other re-work will be needed.

haslinghuis · 2021-12-20T22:34:56Z

Sonar disabled us here from merging this 😕

haslinghuis requested changes Sep 10, 2021

View reviewed changes

haslinghuis added the RN: IMPROVEMENT label Sep 10, 2021

haslinghuis added this to the 10.8.0 milestone Sep 10, 2021

haslinghuis added the Needs coordination with firmware label Sep 10, 2021

TonyBlit force-pushed the gps_sat_list_extra branch from 03b1f2a to 8718685 Compare September 11, 2021 18:20

haslinghuis reviewed Sep 13, 2021

View reviewed changes

TonyBlit force-pushed the gps_sat_list_extra branch from 53def13 to 488c66f Compare September 15, 2021 06:17

TonyBlit force-pushed the gps_sat_list_extra branch from 488c66f to 33783c4 Compare September 15, 2021 06:21

McGiverGim previously approved these changes Sep 15, 2021

View reviewed changes

haslinghuis previously approved these changes Sep 15, 2021

View reviewed changes

TonyBlit dismissed stale reviews from haslinghuis and McGiverGim via deea9f9 September 18, 2021 06:25

haslinghuis previously approved these changes Sep 18, 2021

View reviewed changes

McGiverGim previously approved these changes Sep 20, 2021

View reviewed changes

ctzsnooze previously approved these changes Oct 12, 2021

View reviewed changes

limonspb requested changes Nov 25, 2021

View reviewed changes

TonyBlit dismissed stale reviews from ctzsnooze, McGiverGim, and haslinghuis via be70e00 November 25, 2021 07:21

TonyBlit force-pushed the gps_sat_list_extra branch from 40bcd55 to be70e00 Compare November 25, 2021 07:21

limonspb previously approved these changes Nov 25, 2021

View reviewed changes

haslinghuis previously approved these changes Nov 25, 2021

View reviewed changes

limonspb requested review from McGiverGim and ctzsnooze November 25, 2021 12:53

blckmn approved these changes Dec 3, 2021

View reviewed changes

blckmn previously approved these changes Dec 3, 2021

View reviewed changes

McGiverGim previously approved these changes Dec 3, 2021

View reviewed changes

haslinghuis self-assigned this Dec 3, 2021

ctzsnooze previously approved these changes Dec 13, 2021

View reviewed changes

chmelevskij reviewed Dec 20, 2021

View reviewed changes

src/js/tabs/gps.js Outdated Show resolved Hide resolved

chmelevskij previously approved these changes Dec 20, 2021

View reviewed changes

enhanced sat list for M8N/M9N

cbfeb25

TonyBlit dismissed stale reviews from chmelevskij, ctzsnooze, McGiverGim, blckmn, haslinghuis, and limonspb via cbfeb25 December 20, 2021 07:04

TonyBlit force-pushed the gps_sat_list_extra branch from be70e00 to cbfeb25 Compare December 20, 2021 07:04

chmelevskij approved these changes Dec 20, 2021

View reviewed changes

ctzsnooze approved these changes Dec 20, 2021

View reviewed changes

limonspb approved these changes Dec 20, 2021

View reviewed changes

asizon requested changes Dec 20, 2021

View reviewed changes

asizon approved these changes Dec 20, 2021

View reviewed changes

blckmn approved these changes Dec 20, 2021

View reviewed changes

limonspb merged commit b396f0a into betaflight:master Dec 20, 2021

	quality = quality + 'no signal \| ';
	quality += 'no signal \| ';

Uh oh!

Enhanced sat list for M8N/M9N #2603

Enhanced sat list for M8N/M9N #2603

Uh oh!

Conversation

TonyBlit commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haslinghuis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haslinghuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

McGiverGim commented Sep 14, 2021

Uh oh!

TonyBlit commented Sep 15, 2021

Uh oh!

McGiverGim left a comment

Choose a reason for hiding this comment

Uh oh!

haslinghuis left a comment

Choose a reason for hiding this comment

Uh oh!

McGiverGim commented Sep 17, 2021

Uh oh!

TonyBlit commented Sep 18, 2021

Uh oh!

sonarqubecloud bot commented Sep 18, 2021

Uh oh!

limonspb left a comment

Choose a reason for hiding this comment

Uh oh!

blckmn commented Dec 3, 2021

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 20, 2021

Uh oh!

asizon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmelevskij Dec 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haslinghuis commented Dec 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TonyBlit commented Sep 9, 2021 •

edited

Loading

haslinghuis left a comment •

edited

Loading

asizon left a comment •

edited

Loading

chmelevskij Dec 20, 2021 •

edited

Loading