Add an VTX tab #1569

McGiverGim · 2019-08-09T13:39:13Z

Adds an VTX tab to control the VTX and populate the VTX Table. To go with: betaflight/betaflight#8693

This PR is not finished, it needs some refactor and some little changes, but is at 99% almost done. It has been opened to discuss some things:

Now, the tab only appears for Betaflight 4.1, but the code to make it work in Betaflight 4.0 (or earlier) is almost there if we want to give support because the VTX Table is conditional at compilation so I needed to take it into account.
The tab is divided into two, the upper part can be used in older Betafligth versions but it can be used too in 4.1 when there is no vtx table support.
In the upper right there is a box with general info that is valid always.
Flag vtxtype unknown, vtxtype not supported. We want to show the tab only with the general info and with a message that VTX not valid? Or we prefer to hide the tab?
Flag vtxtables not supported. We want to show the tab only with the upper part giving some functionality or hide the tab?

McGiverGim · 2019-08-09T15:07:38Z

Little reordering of fields, I think is uglier, but now is like the output of vtxtable:

Docteh · 2019-08-09T19:53:04Z

Does this get us the ability to configure everything VTX related within the configurator?

Would the VTX tab showing up be conditional on a port specifying either Smart Audio or Tramp? If that is the case we might want to have the ports information grabbed on connect

McGiverGim · 2019-08-09T21:13:46Z

Yes, that's the idea. Configure all the VTX from within the Configurator.
About it showing up, is the decision that has to be made.

locales/en/messages.json

src/js/msp/MSPHelper.js

mikeller · 2019-08-10T06:20:14Z

This looks good, thank you very much @McGiverGim.

I think we should also look into adding a way to easily load a set of values into this tab from a file / snippet, so that manufacturers can publish VTX tables suitable for their hardware.

McGiverGim · 2019-08-10T06:29:19Z

Yes, my idea is add later a wizard that adds sample values to all fields. But I prefer to do it in a later PR.

mikeller · 2019-08-10T21:50:38Z

Yes, my idea is add later a wizard that adds sample values to all fields.

As in 'sample values loaded from a file'?

We have to avoid putting any 'default' or 'sample' values into the firmware or configurator, as this will open us up to being accused of enabling users to break the law, and potentially be banned from distribution.

McGiverGim · 2019-08-12T06:27:29Z

Yes, that is the reason why I didn't add them here. I prefer to discuss later which is the best way to do it.

My idea was to add a wizard, where the user can select the "sample" bands to use, and the wizard will complete the VTX Table, showing a message that "the user must verify that the bands are valid for the country and adjust them to the law before saving". But as I say, is better to discuss it. We have samples too in the VTX Table wiki page, so I don't think this is very different for that.

Docteh · 2019-08-12T06:56:52Z

Well, with the samples over on the wiki, a user has to leave the configurator and go over there.

I can kind of see this going both ways, if its made too difficult to get the right sort of information, users will end up just internet searching the best thing to shove in, which might have all channels unlocked.

It sounds like FCC and CEPT might have clear limitations, the thing to avoid would be a third option that just unlocks everything.

If we get a nastygram on a wiki page, we just have to remove/correct some information, correcting a configurator would be a little more work.

McGiverGim · 2019-08-12T07:42:32Z

Force pushed changes:

Fixed code as per review
Modified with MSP changes in the PR of firmware part
Hide the tables and show a warning note when vtx not supported or unknown.
Little corrections in CSS and HTML.

I think is more or less ready to go. Any remaining detail can be fixed later if needed.

McGiverGim · 2019-08-12T08:11:21Z

Rebased too with latest master...

McGiverGim · 2019-08-13T08:56:28Z

Pushed a little update: animated the hide and show of the frequency vs channel/band selection. It seems to me that it was too hard the change :)

etracer65 · 2019-08-13T12:54:27Z

Was doing some testing this morning and there are some problems with how the "Enter frequency directly" logic is working. The logic is that if band > 0 and frequency > 0 then we're in "enter directly" mode. Otherwise we're in band/channel mode.

The data that's being sent in MSP_SET_VTX_CONFIG needs to follow this as well so the firmware knows how to behave. So if in "enter directly" mode then band must be 0.

etracer65 · 2019-08-13T12:56:08Z

Also I pushed a small fix to the MSP PR that properly does the frequency lookup based on the supplied band/channel. Nothing that affects your changes - just fixes the display only the right panel after a save so the frequency displays the actual value rather than 0. Previously required a reboot to get the frequency set properly.

etracer65 · 2019-08-13T12:57:00Z

Finally, one cosmetic suggestion. It loos like the band and channel count entries could be located on the same row to save a little vertical space.

McGiverGim · 2019-08-13T13:28:49Z

Was doing some testing this morning and there are some problems with how the "Enter frequency directly" logic is working. The logic is that if band > 0 and frequency > 0 then we're in "enter directly" mode. Otherwise we're in band/channel mode.

Did you mean band = 0 and frequency > 0? If not I don't understand that.

Finally, one cosmetic suggestion. It loos like the band and channel count entries could be located on the same row to save a little vertical space.

Yes, and the power level too, but this breaks a little the fields size aesthetic of all the tab and it will not serve as nothing, the table will end using the same space: the VTX Table has more width than the mode selection table, and we have the status table in the right. I can't move up the VTX Table because it will go over the status. So is better than the mode table has a similar height than the status table from a design point of view.

etracer65 · 2019-08-13T13:31:10Z

Yeah, sorry. I meant band == 0 && frequency > 0. I originally wrote it the reverse describing band/frequency mode and then edited it incorrectly to describe "direct entry" mode.

etracer65 · 2019-08-13T13:34:23Z

I was referring to the band/channel counts in the VTX Table section at the bottom.

McGiverGim · 2019-08-13T13:35:34Z

Yeah, sorry. I meant band == 0 && frequency > 0. I originally wrote it the reverse describing band/frequency mode and then edited it incorrectly to describe "direct entry" mode.

Then maybe I have a bug that I can't see. This is what I tried to do here:

betaflight-configurator/src/js/tabs/vtx.js

Lines 405 to 419 in 1925fe0

    
           let frequencyEnabled = $("vtx_frequency_channel").prop('checked'); 
        
           if (frequencyEnabled) { 
        
               VTX_CONFIG.vtx_frequency = parseInt($("#vtx_frequency").val()); 
        
               VTX_CONFIG.vtx_band = 0; 
        
               VTX_CONFIG.vtx_channel = 0; 
        
           } else { 
        
               VTX_CONFIG.vtx_band = parseInt($("#vtx_band").val()); 
        
               VTX_CONFIG.vtx_channel = parseInt($("#vtx_channel").val()); 
        
               VTX_CONFIG.vtx_frequency = 0; 
        
               if (semver.lt(CONFIG.apiVersion, "1.42.0")) { 
        
                   if (VTX_CONFIG.vtx_band > 0 || VTX_CONFIG.vtx_channel > 0) { 
        
                       VTX_CONFIG.vtx_frequency = (band - 1) * 8 + (channel - 1); 
        
                   } 
        
               } 
        
           }

When the direct frequency is enabled, I put the band and channel to zero. If channel/frequency mode enable, I put the frequency to zero except if firmware is less that 1.42, then the frequency has the codification of band and channel.

If you have found a bug that I can reproduce please describe the steps.

etracer65 · 2019-08-13T13:48:19Z

First test:

Set up some value vtx table info - doesn't really matter what. I used:

# vtxtable
vtxtable bands 5
vtxtable channels 8
vtxtable band 1 BOSCAM_A A CUSTOM  5865 5845 5825 5805 5785 5765 5745 5725
vtxtable band 2 BOSCAM_B B CUSTOM  5733 5752 5771 5790 5809 5828 5847 5866
vtxtable band 3 BOSCAM_E E CUSTOM  5705 5685 5665 5645 5885 5905 5925 5945
vtxtable band 4 FATSHARK F CUSTOM  5740 5760 5780 5800 5820 5840 5860 5880
vtxtable band 5 RACEBAND R CUSTOM  5658 5695 5732 5769 5806 5843 5880 5917
vtxtable powerlevels 3
vtxtable powervalues 25 200 400
vtxtable powerlabels 25 200 500

Then in CLI set it up for a manual frequency:

set vtx_band = 0
set vtx_freq = 5800

After saving and going back to the tab it will have:

etracer65 · 2019-08-13T13:51:25Z

Next test: Start with the same config but this time set the band and channel like:

set vtx_band = 5
set vtx_channel = 1

(or use the Configurator tab).

The tab will be correct and look like this:

Then change to a manual frequency using the switch and try to set to 5800 like this:

Then save and the settings will revert back to the band/channel setup.

McGiverGim · 2019-08-13T13:53:49Z

If you are referring at the state of the switch, I don't have a manner to know if the values that I'm reading are from a frequency or from a band/channel selection. I can modify it "remembering" the latest state, but nothing more. Is that what you suggest?

etracer65 · 2019-08-13T14:04:41Z

If you are referring at the state of the switch, I don't have a manner to know if the values that I'm reading are from a frequency or from a band/channel selection. I can modify it "remembering" the latest state, but nothing more. Is that what you suggest?

When you read the settings from MSP_VTX_CONFIG, if band == 0 and frequency > 0 then the "enter directly" switch should be selected. Otherwise not and the user has to enter band and channel. So no, not remembering from client side. Basing it on the state of the settings.

McGiverGim · 2019-08-13T14:43:04Z

Sorry for the two forces, in the first there were some code that I don't wanted to push.

@etracer65 Fixed the problem, there was a bug in the set part.

Another think: the frequency value comes with zero when there is a band/channel selected. I can see it at the status in the right. It will not be interesting to populate it with the real frequency from the table? If yes, this can be done in the firmware part or the Configurator part.

etracer65 · 2019-08-13T14:49:06Z

Another think: the frequency value comes with zero when there is a band/channel selected. I can see it at the status in the right. It will not be interesting to populate it with the real frequency from the table? If yes, this can be done in the firmware part or the Configurator part.

That's already fixed and is the update to the MSP PR that I mentioned earlier today.

McGiverGim · 2019-08-13T14:51:23Z

Perfect then! We are ready to go with a first version :)

etracer65 · 2019-08-13T16:30:52Z

Just tested the "direct entry" vs. the band/channel entry and it now looks good. The cosmetic improvement I was referring to was this:

Seems like we could put these both on the same row and save a little vertical space.

McGiverGim · 2019-08-15T07:14:20Z

Sorry for the late response, I didn't have time before. Implemented the @etracer65 suggestion:

mikeller

This looks good, and works well, thank you very much.

One thing that needs to be fixed is that it should not be possible to set Band / Channel in 'Selected Mode' to a combination if the corresponding field in the VTX table contains 0 It is probably easiest if we use a drop down for the 'Channel' setting to make this possible.

I like how the sections are arranged in the tab. But I think that, because the 'VTX Table' section is below the 'Selected Mode' section, it is not obvious to users that they first need to configure 'VTX Table' before they can configure 'Selected Mode'. I think to make it easier for users to realise this, it would be helpful if we showed a note about this at the top of the 'Selected Mode' section whenever the 'VTX Table' section is unconfigured.

McGiverGim · 2019-08-16T06:25:18Z

One thing that needs to be fixed is that it should not be possible to set Band / Channel in 'Selected Mode' to a combination if the corresponding field in the VTX table contains 0 It is probably easiest if we use a drop down for the 'Channel' setting to make this possible.

Do you want that this works against the stored VTX Table or against the "live" vtx table? Now, the upper table works against the stored values, I can change it to make it work against the "live" table, but the values of the upper table must be sent before the new vtx table, so I don't think if it has sense. It will be enough to filter against the stored VTX Table of do I change it to be against the "live" table?

I think to make it easier for users to realise this, it would be helpful if we showed a note about this at the top of the 'Selected Mode' section whenever the 'VTX Table' section is unconfigured.

Ok, I will add it.

mikeller · 2019-08-16T07:27:43Z

One thing that @etracer65 suggested was to, instead of limiting the choices for band / channel selection, show a warning informing users that the band / channel they selected was disabled - this would be easy to be made to work against the to be stored VTX table, and it would work for what we want.

McGiverGim · 2019-08-16T07:34:00Z

I'm not too sure how to implement that, to me is the same. If I verify if the band/channel exist to show a warning, I must do it against the stored or the live table.

Maybe are you talking about:

Let the user select any band channel from 1 to 8.
Let the user save it.
If the band/channel are zero when reading it again from MSP, then they were invalid. Show a warning.

I think this makes the things too complicated. I'm not too sure if this will work as expected. The band/channel is sent before the new VTX Table, so maybe is valid but not at this moment so is the same that if I verify them against the stored table.

McGiverGim · 2019-08-16T07:36:35Z

@etracer65 another thing I've observed doing tests is that if I select a band/channel and save it, and then I remove the VTX Table, the band/channel selected continues and I receive it as the band/channel selected. Is that ok?

etracer65 · 2019-08-16T14:51:00Z

@etracer65 another thing I've observed doing tests is that if I select a band/channel and save it, and then I remove the VTX Table, the band/channel selected continues and I receive it as the band/channel selected. Is that ok?

Well it's not ideal, but also not really a big problem. I guess we should probably reset the band/channel (and also powerlevel) back to 0 if they exceed the available vtxtable count. Something like this would be normally done in the config validation in config.c when saving (so that changes from other sources like CLI are also validated), not in the MSP. I'll look at a separate PR since it's not directly related to the MSP one.

etracer65 · 2019-08-16T15:57:36Z

As mentioned, added a separate PR to reset invalid vtx band/channel/powerlevel settings.

betaflight/betaflight#8726

McGiverGim · 2019-08-17T05:47:20Z

Without changing the entire way this tab is designed, and the MSP command is designed I think the better is to validate against the stored values, and if the user modifies the VTX Table letting and invalid value for the selected one, the FC will return it to zero. I think this will be enough.

McGiverGim · 2019-08-17T07:02:26Z

Pushed a PR with:

Some little fixes
Adding a message remembering to fill the VTX Table before use the Select Mode form.
Letting only select the correct channel values stored in the FC.

I think this is the better that we can do without moving the VTX Table to another place (maybe a button that opens the table in a window to modify it, and let a read only table in the tab, but I think this will complicate the user experience to fix some edge cases).

McGiverGim · 2019-08-19T12:00:11Z

It seems I forgot to push some update? I have modified files at my local branch, sorry.

McGiverGim · 2019-08-20T09:31:13Z

@mikeller what is the final decision with this? Do I need to add some warning or similar? Is not clear to me if this has some changes pending.

mikeller

@McGiverGim: Sorry, I didn't get around to testing this until now.
It looks good, good work. One minor remark about the messages.

locales/en/messages.json

McGiverGim · 2019-08-20T11:24:38Z

Added the yes/no general messages... is a little strange because it wasn't used in other places, the only other place was commented, but now we have it ready for the future ;)

McGiverGim added the In Development label Aug 9, 2019

McGiverGim force-pushed the add_vtx_tab branch from 11cb08c to 911010a Compare August 9, 2019 15:09

McGiverGim added the Needs coordination with firmware label Aug 9, 2019

mikeller reviewed Aug 10, 2019

View reviewed changes

locales/en/messages.json Outdated Show resolved Hide resolved

src/js/msp/MSPHelper.js Outdated Show resolved Hide resolved

McGiverGim force-pushed the add_vtx_tab branch from 911010a to ec4cb58 Compare August 12, 2019 07:40

McGiverGim removed the In Development label Aug 12, 2019

McGiverGim force-pushed the add_vtx_tab branch from ec4cb58 to b14f913 Compare August 12, 2019 08:10

McGiverGim force-pushed the add_vtx_tab branch from b14f913 to 1925fe0 Compare August 13, 2019 08:50

McGiverGim force-pushed the add_vtx_tab branch from 50bd7a1 to 5f63c0d Compare August 13, 2019 14:39

McGiverGim force-pushed the add_vtx_tab branch from 5f63c0d to 9f21c67 Compare August 15, 2019 07:13

mikeller requested changes Aug 15, 2019

View reviewed changes

McGiverGim force-pushed the add_vtx_tab branch from 9f21c67 to a0ee760 Compare August 17, 2019 06:59

McGiverGim force-pushed the add_vtx_tab branch from a0ee760 to 11685e1 Compare August 19, 2019 11:58

mikeller reviewed Aug 20, 2019

View reviewed changes

locales/en/messages.json Outdated Show resolved Hide resolved

mikeller added this to the 10.6.0 milestone Aug 20, 2019

mikeller added the RN: IMPROVEMENT label Aug 20, 2019

Add an VTX tab

0817e7d

McGiverGim force-pushed the add_vtx_tab branch from 11685e1 to 0817e7d Compare August 20, 2019 11:23

mikeller approved these changes Aug 20, 2019

View reviewed changes

mikeller merged commit bb67dbb into betaflight:master Aug 20, 2019

mikeller added RN: FEATURE and removed RN: IMPROVEMENT labels Sep 15, 2019

Uh oh!

Add an VTX tab #1569

Add an VTX tab #1569

Uh oh!

Conversation

McGiverGim commented Aug 9, 2019

Uh oh!

McGiverGim commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Docteh commented Aug 9, 2019

Uh oh!

McGiverGim commented Aug 9, 2019

Uh oh!

Uh oh!

Uh oh!

mikeller commented Aug 10, 2019

Uh oh!

McGiverGim commented Aug 10, 2019

Uh oh!

mikeller commented Aug 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

McGiverGim commented Aug 12, 2019

Uh oh!

Docteh commented Aug 12, 2019

Uh oh!

McGiverGim commented Aug 12, 2019

Uh oh!

McGiverGim commented Aug 12, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 13, 2019

Uh oh!

etracer65 commented Aug 13, 2019

Uh oh!

McGiverGim commented Aug 15, 2019

Uh oh!

mikeller left a comment

Choose a reason for hiding this comment

Uh oh!

McGiverGim commented Aug 16, 2019

Uh oh!

mikeller commented Aug 16, 2019

Uh oh!

McGiverGim commented Aug 16, 2019

Uh oh!

McGiverGim commented Aug 16, 2019

Uh oh!

etracer65 commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etracer65 commented Aug 16, 2019

McGiverGim commented Aug 9, 2019 •

edited

Loading

mikeller commented Aug 10, 2019 •

edited

Loading

etracer65 commented Aug 16, 2019 •

edited

Loading