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

Implement 'vtx_' settings and SetFreqByMHzMsp support #4265

Merged
merged 1 commit into from Oct 20, 2017

Conversation

Projects
None yet
6 participants
@ethomas997
Contributor

ethomas997 commented Oct 2, 2017

This PR encompasses the changes for 4 PRs that I've posted on the Cleanflight side, as described below. With these changes, TBS-SmartAudio and IRC-Tramp video transmitters can be configured via CLI settings, and can be directly tuned to a frequency in MHz using a CLI setting or a Taranis/OpenTX transmitter (via smartport telemetry and modified 'lua' script).


Implement 'vtx_' settings for SmartAudio and Tramp
cleanflight/cleanflight#2930
Add 'vtx_freq' setting for SmartAudio and Tramp
cleanflight/cleanflight#2936

This takes the 'vtx_' settings that @hydra created (vtx_band / vtx_channel / vtx_power) for RTC6705 and also implements them for TBS-SmartAudio and IRC-Tramp video transmitters. At startup the settings are applied to the transmitter. If the video setup is modified via the CMS OSD menu or via MSP (Taranis/OpenTX smartport), the settings are updated.

One nice thing the settings can provide is a way to configure a frequency (via USB / CLI) while the video transmitter is not powered up. Afer a save and power cycle, the system will startup at the new frequency.

Also, this adds a 'vtx_freq' setting for TBS-SmartAudio and IRC-Tramp video transmitters (I don't have an RTC6705 / SPRacing NEO board to test with.) If vtx_band=0 and vtx_freq!=0 then the 'vtx_freq' value (in MHz) will be configured on the transmitter at startup. If both are zero then the settings will be ignored. If vtx_band!=0 and a video transmitter is connected then 'vtx_freq' will be set to the current frequency value (in MHz) at startup.

vtx_band = #
Allowed range: 0 - 5
0=user, 1=A, 2=B, 3=E, 4=F(Airwaves/Fatshark), 5=Raceband

vtx_channel = #
Allowed range: 1 - 8

vtx_power = #
Allowed range: 0 - 5
for SmartAudio: 0=25mW, 1=25mW, 2=200mW, 3=500mW, 4=800mW
for TrampHV: 0=25mW, 1=25mW, 2=100mW, 3=200mW, 4=400mW, 5=600mW

vtx_freq = ####
Allowed range: 0 - 5999
if vtx_band!=0 and VTX connected then shows freq in MHz
if vtx_band==0 then sets frequency in MHz

if vtx_band==0 and vtx_freq==0 then the settings will not be sent out to the VTX


SmartAudio-CMS fixes and improvements
cleanflight/cleanflight#2941

Implements some fixes and improvements to the SmartAudio part of the CMS OSD menus:

CMS set of USER frequency would fail if frequency was same as previous USER frequency

CMS would set vtx freq immediately when going from USER to CHAN freq mode
(should wait until SET performed)

After changing CHAN/USER freq mode but before SET, the CMS status line
would show wrong band-channel / U-F display
(needed new 'saCmsFselModeNew' variable)

CMS will now update when saDevice settings change
(if race opmodel via CMS and then user freq set via CLI "set vtx_freq",
freq is set but CMS would report race opmodel and old band/channel)

Modified SA CMS to use 'saSetPitFreq()' function

Moved 'saCmsUpdate()' declaration to 'cms_menu_vtx_smartaudio.h'


Add SetFreqByMHzMsp support for SmartAudio and Tramp
cleanflight/cleanflight#2947

Adds support for setting the VTX frequency in MHz via MSP for TBS-SmartAudio and IRC-Tramp video transmitters.

I've posted modified 'betaflight-tx-lua-scripts' that utilize this new functionality, here:
betaflight/betaflight-tx-lua-scripts#41
https://github.com/ethomas997/betaflight-tx-lua-scripts/releases/tag/v0.4

--ET

@ethomas997 ethomas997 referenced this pull request Oct 2, 2017

Closed

Made vtx freq editable #41

@hydra hydra referenced this pull request Oct 2, 2017

Closed

Add SetFreqByMHzMsp support #2947

@@ -340,9 +355,9 @@ static CMS_Menu saCmsMenuStats = {
.entries = saCmsMenuStatsEntries
};
static OSD_TAB_t saCmsEntBand = { &saCmsBand, 5, vtx58BandNames };
static OSD_TAB_t saCmsEntBand = { &saCmsBand, VTX_SMARTAUDIO_BAND_COUNT, vtx58BandNames };

This comment has been minimized.

@hydra

hydra Oct 2, 2017

Contributor

@ethomas997 good to see this is using driver-specific defines now. 👍

@hydra

hydra Oct 2, 2017

Contributor

@ethomas997 good to see this is using driver-specific defines now. 👍

@@ -1897,7 +1891,7 @@ static void cliVtx(char *cmdline)
if (ptr) {
val = atoi(ptr);
// FIXME Use VTX API to get min/max
if (val >= VTX_BAND_MIN && val <= VTX_BAND_MAX) {
if (val >= VTX_SETTINGS_MIN_BAND && val <= VTX_SETTINGS_MAX_BAND) {

This comment has been minimized.

@hydra

hydra Oct 2, 2017

Contributor

does the VTX API allow the FIXME here and below to be fixed now?

@hydra

hydra Oct 2, 2017

Contributor

does the VTX API allow the FIXME here and below to be fixed now?

This comment has been minimized.

@ethomas997

ethomas997 Oct 8, 2017

Contributor

Maybe, but I think that would want to be via a separate PR and testing.

@ethomas997

ethomas997 Oct 8, 2017

Contributor

Maybe, but I think that would want to be via a separate PR and testing.

Show outdated Hide outdated src/main/fc/fc_msp.c Outdated
Show outdated Hide outdated src/main/fc/fc_msp.c Outdated
Show outdated Hide outdated src/main/fc/fc_msp.c Outdated
@mikeller

Looks good to me, except for the points @hydra commented on.

Show outdated Hide outdated src/main/io/vtx_smartaudio.c Outdated
Show outdated Hide outdated src/main/io/vtx_smartaudio.c Outdated
@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Oct 2, 2017

Contributor

overall this PR is great, quite a few trivial style cleanups to fix and magic numbers to replace with #defines.

@ethomas997 can you fix them with additional/replacement commits (add-to or update this PR branch)?

Can you also confirm that you have tested the changes on:

  • RTC6705
  • SA V1 device
  • SA V2 device
  • Tramp
Contributor

hydra commented Oct 2, 2017

overall this PR is great, quite a few trivial style cleanups to fix and magic numbers to replace with #defines.

@ethomas997 can you fix them with additional/replacement commits (add-to or update this PR branch)?

Can you also confirm that you have tested the changes on:

  • RTC6705
  • SA V1 device
  • SA V2 device
  • Tramp
@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 2, 2017

Contributor

@hydra Thanks for reviewing it; yes, I'll do those changes. For hardware to test with I have a "TBS Unify Pro HV Race (SA V2)", a "TBS Unify Pro HV V2" and a "Tramp HV". (I don't have anything that's SA V1.)

Most of the new functionality is not implemented for RTC6705 / SPR-NEO. The current RTC6705 implementation would need some reworking to support setting frequency in MHz. I'm willing to work on it, but I would need to get my hands on compatible hardware.

Contributor

ethomas997 commented Oct 2, 2017

@hydra Thanks for reviewing it; yes, I'll do those changes. For hardware to test with I have a "TBS Unify Pro HV Race (SA V2)", a "TBS Unify Pro HV V2" and a "Tramp HV". (I don't have anything that's SA V1.)

Most of the new functionality is not implemented for RTC6705 / SPR-NEO. The current RTC6705 implementation would need some reworking to support setting frequency in MHz. I'm willing to work on it, but I would need to get my hands on compatible hardware.

// band: Band value (1 to 5).
// channel: Channel value (1 to 8).
// index: Power-index value.
void vtxSettingsSaveBandChanAndPower(uint8_t band, uint8_t channel, uint8_t index)

This comment has been minimized.

@martinbudden

martinbudden Oct 3, 2017

Contributor

This function should be removed. It's usage should be replaced by a call to vtxSettingsSaveBandAndChannel followed by a call to vtxSettingsSaveBandAndChannel.

@martinbudden

martinbudden Oct 3, 2017

Contributor

This function should be removed. It's usage should be replaced by a call to vtxSettingsSaveBandAndChannel followed by a call to vtxSettingsSaveBandAndChannel.

This comment has been minimized.

@ethomas997

ethomas997 Oct 8, 2017

Contributor

Don't want to have multiple calls to saveConfigAndNotify().

@ethomas997

ethomas997 Oct 8, 2017

Contributor

Don't want to have multiple calls to saveConfigAndNotify().

}
if (modFlag) {
// need to save config so vtx settings in place after reboot
saveConfigAndNotify();

This comment has been minimized.

@martinbudden

martinbudden Oct 3, 2017

Contributor

The call to saveConfigAndNotify should not be made here - it should be handled at a higher level if and when required. Generally set functions should not have side effects.

@martinbudden

martinbudden Oct 3, 2017

Contributor

The call to saveConfigAndNotify should not be made here - it should be handled at a higher level if and when required. Generally set functions should not have side effects.

This comment has been minimized.

@ethomas997

ethomas997 Oct 8, 2017

Contributor

I'm not seeing a way to move the saveConfigAndNotify() elsewhere that doesn't get very messy.

@ethomas997

ethomas997 Oct 8, 2017

Contributor

I'm not seeing a way to move the saveConfigAndNotify() elsewhere that doesn't get very messy.

Show outdated Hide outdated src/main/io/vtx_tramp.h Outdated
Show outdated Hide outdated src/main/io/vtx_tramp.h Outdated
@martinbudden

This comment has been minimized.

Show comment
Hide comment
@martinbudden

martinbudden Oct 3, 2017

Contributor

@ethomas997 , overall I like these changes.

One thing you have changes is how the VTX config works - if the config is changed then saveConfigAndNotify is called so changes are saved to the EEPROM. This makes VTX config different to other config settings - what is the underlying reason for this change?

Contributor

martinbudden commented Oct 3, 2017

@ethomas997 , overall I like these changes.

One thing you have changes is how the VTX config works - if the config is changed then saveConfigAndNotify is called so changes are saved to the EEPROM. This makes VTX config different to other config settings - what is the underlying reason for this change?

@martinbudden martinbudden added this to the Betaflight v3.3 milestone Oct 3, 2017

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 3, 2017

Contributor

@martinbudden The VTX configuration settings are applied at startup, so they need to be saved if modified via CMS or MSP.

Contributor

ethomas997 commented Oct 3, 2017

@martinbudden The VTX configuration settings are applied at startup, so they need to be saved if modified via CMS or MSP.

@martinbudden

This comment has been minimized.

Show comment
Hide comment
@martinbudden

martinbudden Oct 3, 2017

Contributor

@ethomas997 , I understand they need to be save to take effect next startup. However this can be handled at a higher level, ie in CMS or MSP - this is how it is done for other configs. Eg cmsMenuExit allows user to save or not - so they have option of abandoning changes they have made.

Contributor

martinbudden commented Oct 3, 2017

@ethomas997 , I understand they need to be save to take effect next startup. However this can be handled at a higher level, ie in CMS or MSP - this is how it is done for other configs. Eg cmsMenuExit allows user to save or not - so they have option of abandoning changes they have made.

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Oct 3, 2017

Contributor

@ethomas997 i can supply you with F3 and F4 NEO to test with. Drop me an email with your name and address and I'll get my assistant to dispatch one of each to you.

Contributor

hydra commented Oct 3, 2017

@ethomas997 i can supply you with F3 and F4 NEO to test with. Drop me an email with your name and address and I'll get my assistant to dispatch one of each to you.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 3, 2017

Contributor

@martinbudden My implementation preserves the pre-existing functionality, which is that VTX changes in CMS or via MSP are persistent and don't need to be explicitly saved. I think it's best that it operates this way. If a pilot is in a group setting and the VTX frequency changes (unexpectedly) after reboot, that would be bad.

Contributor

ethomas997 commented Oct 3, 2017

@martinbudden My implementation preserves the pre-existing functionality, which is that VTX changes in CMS or via MSP are persistent and don't need to be explicitly saved. I think it's best that it operates this way. If a pilot is in a group setting and the VTX frequency changes (unexpectedly) after reboot, that would be bad.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 8, 2017

Contributor

I've posted modifications in response to PR comments. Did testing with TBS-SmartAudio (V2) and IRC-Tramp video transmitters.

Contributor

ethomas997 commented Oct 8, 2017

I've posted modifications in response to PR comments. Did testing with TBS-SmartAudio (V2) and IRC-Tramp video transmitters.

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Oct 9, 2017

Contributor

its simple enough to remove the call to saveConfigAndNotify and let the user save settings using the stick commands to save the config.

Contributor

hydra commented Oct 9, 2017

its simple enough to remove the call to saveConfigAndNotify and let the user save settings using the stick commands to save the config.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 9, 2017

Contributor

@hydra I think that would be a significant change to how the CMS VTX menus currently operate. Right now the user does the SET operation, the VTX config changes, and the config change is persistent.

The other thing is the config via MSP / Taranis-lua-script. Similarly, the user enters the config change and it is persistent.

Contributor

ethomas997 commented Oct 9, 2017

@hydra I think that would be a significant change to how the CMS VTX menus currently operate. Right now the user does the SET operation, the VTX config changes, and the config change is persistent.

The other thing is the config via MSP / Taranis-lua-script. Similarly, the user enters the config change and it is persistent.

@martinbudden

This comment has been minimized.

Show comment
Hide comment
@martinbudden

martinbudden Oct 9, 2017

Contributor

@ethomas997 , agreed, this PR shouldn't change the behaviour WRT persistency.

Contributor

martinbudden commented Oct 9, 2017

@ethomas997 , agreed, this PR shouldn't change the behaviour WRT persistency.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 9, 2017

Contributor

@martinbudden Yeah, it's tricky because the video transmitters have their own persistent saving of configuration. This was the best way I could see for handing the settings.

Contributor

ethomas997 commented Oct 9, 2017

@martinbudden Yeah, it's tricky because the video transmitters have their own persistent saving of configuration. This was the best way I could see for handing the settings.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 10, 2017

Contributor

I've posted firmware test builds and documentation for this mod here: http://www.etheli.com/CF/vtxCfgCMSFreqViaMsp

Contributor

ethomas997 commented Oct 10, 2017

I've posted firmware test builds and documentation for this mod here: http://www.etheli.com/CF/vtxCfgCMSFreqViaMsp

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 19, 2017

Contributor

@hydra I sent you an email with my name & address on 10/3; did you receive it?

AFAIK this PR is ready.

Contributor

ethomas997 commented Oct 19, 2017

@hydra I sent you an email with my name & address on 10/3; did you receive it?

AFAIK this PR is ready.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Oct 19, 2017

Member

@ethomas997: There are still some change requests from @martinbudden open. Also, I have noticed that you are using an _ prefix for function names, which is not commonly done in the Betaflight codebase. Please change.

Member

mikeller commented Oct 19, 2017

@ethomas997: There are still some change requests from @martinbudden open. Also, I have noticed that you are using an _ prefix for function names, which is not commonly done in the Betaflight codebase. Please change.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 20, 2017

Contributor

@mikeller Alright, requested name changes are in place.

If there are other changes needed, please be specific. The last post I see from @martinbudden was to express agreement that the PR shouldn't change the behavior in terms of persistency.

Contributor

ethomas997 commented Oct 20, 2017

@mikeller Alright, requested name changes are in place.

If there are other changes needed, please be specific. The last post I see from @martinbudden was to express agreement that the PR shouldn't change the behavior in terms of persistency.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Oct 20, 2017

Member

@ethomas997: Na, it was just the naming consistency thing. All good to go now.

Member

mikeller commented Oct 20, 2017

@ethomas997: Na, it was just the naming consistency thing. All good to go now.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 20, 2017

Contributor

Should I squish the three commits into one?

Contributor

ethomas997 commented Oct 20, 2017

Should I squish the three commits into one?

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Oct 20, 2017

Member

Yes please.

Member

mikeller commented Oct 20, 2017

Yes please.

Implemented 'vtx_' settings and SetFreqByMHzMsp support
Implemented 'vtx_' settings for SmartAudio and Tramp

Added 'vtx_freq' setting for SmartAudio and Tramp

SmartAudio-CMS fixes and improvements

Added SetFreqByMHzMsp support for SmartAudio and Tramp
@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Oct 20, 2017

Contributor

@mikeller Alright, made into single commit and rebased.

Contributor

ethomas997 commented Oct 20, 2017

@mikeller Alright, made into single commit and rebased.

@mikeller mikeller merged commit 97c3e4b into betaflight:master Oct 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HFMan

This comment has been minimized.

Show comment
Hide comment
@HFMan

HFMan Oct 23, 2017

The power setting for my setup is not being restored (or perhaps saved) correctly. Whenever I save via OSD my desired power, the next time the board powers up the power is set back to 25mW. It doesn't matter what power level I have chosen when I save.
Board is OMNIBUSF4, VTx is an IRC Tramp, firmware is Betaflight build 8a27c9e . I have verified that changing the VTx settings via OSD is working correctly. I have verified that this problem started occurring at exactly when this Pull was built (i.e. later commits aren't causing the issue).

HFMan commented Oct 23, 2017

The power setting for my setup is not being restored (or perhaps saved) correctly. Whenever I save via OSD my desired power, the next time the board powers up the power is set back to 25mW. It doesn't matter what power level I have chosen when I save.
Board is OMNIBUSF4, VTx is an IRC Tramp, firmware is Betaflight build 8a27c9e . I have verified that changing the VTx settings via OSD is working correctly. I have verified that this problem started occurring at exactly when this Pull was built (i.e. later commits aren't causing the issue).

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Oct 23, 2017

Member

@HFMan: Yes, there is a bug with VTX settings being reset to defaults. A fix is currently being worked on.

Member

mikeller commented Oct 23, 2017

@HFMan: Yes, there is a bug with VTX settings being reset to defaults. A fix is currently being worked on.

@HFMan

This comment has been minimized.

Show comment
Hide comment
@HFMan

HFMan Oct 24, 2017

@mikeller: Thanks- is there an Issue# for this? I haven't been able to locate it.

HFMan commented Oct 24, 2017

@mikeller: Thanks- is there an Issue# for this? I haven't been able to locate it.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Oct 24, 2017

Member

@HFMan: I don't think so. Can you create one please?

Member

mikeller commented Oct 24, 2017

@HFMan: I don't think so. Can you create one please?

@AndersHoglund

This comment has been minimized.

Show comment
Hide comment
@AndersHoglund

AndersHoglund Oct 24, 2017

Contributor

This PR introduces a VTX device independant API to Frequency settings. Very good. Howabout doing the same for power? Pitmode is already fairly device independant, actually quite hard to make ON/OFF brand specific. Other than inverting it....

A generic power API could be done using a 16 bit arg for power in mW, 0 ... 65000mW would be enough range. This arg should be interpreted as the max allowed power, and the VTX device should set the closest lower power level.

One question on the Frequency API vs Band/Channel API. In the first post there are some dependencies described that I do not like. For instance setting band=0 and ch=0 will lock the Freq API. Is this still in place? If so, why this complication?? I could read the code, but I though i'd ask here.

Contributor

AndersHoglund commented Oct 24, 2017

This PR introduces a VTX device independant API to Frequency settings. Very good. Howabout doing the same for power? Pitmode is already fairly device independant, actually quite hard to make ON/OFF brand specific. Other than inverting it....

A generic power API could be done using a 16 bit arg for power in mW, 0 ... 65000mW would be enough range. This arg should be interpreted as the max allowed power, and the VTX device should set the closest lower power level.

One question on the Frequency API vs Band/Channel API. In the first post there are some dependencies described that I do not like. For instance setting band=0 and ch=0 will lock the Freq API. Is this still in place? If so, why this complication?? I could read the code, but I though i'd ask here.

@AndersHoglund

This comment has been minimized.

Show comment
Hide comment
@AndersHoglund

AndersHoglund Oct 24, 2017

Contributor

Ah, sorry. I misread the first post. Band needs to be set to USER (0) to be able to set frequency. No dependency to channel.

Contributor

AndersHoglund commented Oct 24, 2017

Ah, sorry. I misread the first post. Band needs to be set to USER (0) to be able to set frequency. No dependency to channel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment