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

Make vtx freq editable - revised #92

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
4 participants
@ethomas997
Contributor

ethomas997 commented Nov 20, 2017

This modifies the VTX script to support editing the 'frequency' field, so it will step through the available frequencies for bands / channels. Implements the feature suggested by issue #33. This is a rebase of PR #41.

Also, if Cleanflight/Betaflight has 'SetFreqByMHzMsp' support then any frequency in MHz can be set when the band is set to 'U'. (The channel field may be used to set the frequency in 100-MHz increments.)

I've posted build files for a devel/beta build (betaflight/betaflight@a9105e8) of Betaflight with 'SetFreqByMHzMsp' support, here: http://www.etheli.com/CF/vtxCfgCMSFreqViaMsp/bf_builds_20171118_372

I've developed and tested this on a Taranis X7; testing on the other transmitter types would be good.

I've tested with Betaflight v3.2.2 and with the 2017-11-18 snapshot of Betaflight mentioned above.

The 'vtx.lua' files for X7/X9/Horus are identical except for the 'text' and 'fields' definitions, so most of the content could be broken out into a 'vtx_common.lua' file.

--ET

@ethomas997 ethomas997 referenced this pull request Nov 20, 2017

Closed

Made vtx freq editable #41

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 20, 2017

Member

I've tested with Betaflight v3.2.2 and with the 2017-11-18 snapshot of Betaflight mentioned above.

So this works with the released version of betaflight 3.2.2?

Member

mikeller commented Nov 20, 2017

I've tested with Betaflight v3.2.2 and with the 2017-11-18 snapshot of Betaflight mentioned above.

So this works with the released version of betaflight 3.2.2?

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 20, 2017

Contributor

@mikeller: With BF v3.2.2, setting via band/channel should work the same as with previous versions of the VTX script. The PR adds the ability to set band/channel by editing the value of the 'frequency' field -- as it is modified, the band/channel change to match the value.

If a recent devel / 'master' version of Betaflight is used, a band value of 'U' may be set and then any frequency value in MHz can be dialed in and set.

Contributor

ethomas997 commented Nov 20, 2017

@mikeller: With BF v3.2.2, setting via band/channel should work the same as with previous versions of the VTX script. The PR adds the ability to set band/channel by editing the value of the 'frequency' field -- as it is modified, the band/channel change to match the value.

If a recent devel / 'master' version of Betaflight is used, a band value of 'U' may be set and then any frequency value in MHz can be dialed in and set.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 20, 2017

Member

Ah I see. How is the case of RTC6705 handled? This driver does not support direct frequency entry.

Member

mikeller commented Nov 20, 2017

Ah I see. How is the case of RTC6705 handled? This driver does not support direct frequency entry.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 20, 2017

Contributor

@mikeller: I don't think any version of the VTX script has supported RTC6705. Only SA and Tramp.

Contributor

ethomas997 commented Nov 20, 2017

@mikeller: I don't think any version of the VTX script has supported RTC6705. Only SA and Tramp.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 20, 2017

Member

@ethomas997: Oh, why is that? I thought we moved to unified drivers for all 3 on the firmware side? @codecae?

Member

mikeller commented Nov 20, 2017

@ethomas997: Oh, why is that? I thought we moved to unified drivers for all 3 on the firmware side? @codecae?

@codecae

This comment has been minimized.

Show comment
Hide comment
@codecae

codecae Nov 20, 2017

Member

The vtable is registered with vtx common on the firmware side, but it doesn't share the same feature set. No pit mode and user-defined frequency.

Member

codecae commented Nov 20, 2017

The vtable is registered with vtx common on the firmware side, but it doesn't share the same feature set. No pit mode and user-defined frequency.

@raphaelcoeffic

This comment has been minimized.

Show comment
Hide comment
@raphaelcoeffic

raphaelcoeffic Nov 20, 2017

Member

I guess the VTX type is not displayed properly, but that should be all.

Member

raphaelcoeffic commented Nov 20, 2017

I guess the VTX type is not displayed properly, but that should be all.

@codecae

This comment has been minimized.

Show comment
Hide comment
@codecae

codecae Nov 20, 2017

Member

I would assume this as well, but do not have hardware to verify.

Member

codecae commented Nov 20, 2017

I would assume this as well, but do not have hardware to verify.

@raphaelcoeffic

This comment has been minimized.

Show comment
Hide comment
@raphaelcoeffic

raphaelcoeffic Nov 20, 2017

Member

neither do I. Could anyone try this out?

Member

raphaelcoeffic commented Nov 20, 2017

neither do I. Could anyone try this out?

Show outdated Hide outdated src/SCRIPTS/BF/HORUS/vtx.lua Outdated
Show outdated Hide outdated src/SCRIPTS/BF/HORUS/vtx.lua Outdated
@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 20, 2017

Contributor

@codecae: Is there a "philosophy" behind when to use "self.fields[x].value" vs "self.values[x]"?

Contributor

ethomas997 commented Nov 20, 2017

@codecae: Is there a "philosophy" behind when to use "self.fields[x].value" vs "self.values[x]"?

@codecae

This comment has been minimized.

Show comment
Hide comment
@codecae

codecae Nov 20, 2017

Member

I don't think there's a philosophy, persay. Generally speaking, I think the intent behind 'fields' is to give more free-form flexibility to place things in locations that are unbound to a value cursor, where specifying configurations on values will bind the heading with the cursor and display them together.

Member

codecae commented Nov 20, 2017

I don't think there's a philosophy, persay. Generally speaking, I think the intent behind 'fields' is to give more free-form flexibility to place things in locations that are unbound to a value cursor, where specifying configurations on values will bind the heading with the cursor and display them together.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 20, 2017

Member

@codecae, @raphaelcoeffic: I have a board with an onboard VTX, I can test if the scripts work with the RTC6705.

Member

mikeller commented Nov 20, 2017

@codecae, @raphaelcoeffic: I have a board with an onboard VTX, I can test if the scripts work with the RTC6705.

@raphaelcoeffic

This comment has been minimized.

Show comment
Hide comment
@raphaelcoeffic

raphaelcoeffic Nov 20, 2017

Member

Is there a "philosophy" behind when to use "self.fields[x].value" vs "self.values[x]"?

@ethomas997 at the beginning, only Page.values[x] was used, as this is where the data are loaded when coming from the FC, and where they are taken when sending back to the FC. The values in the fields are basically derivates to be displayed.

Member

raphaelcoeffic commented Nov 20, 2017

Is there a "philosophy" behind when to use "self.fields[x].value" vs "self.values[x]"?

@ethomas997 at the beginning, only Page.values[x] was used, as this is where the data are loaded when coming from the FC, and where they are taken when sending back to the FC. The values in the fields are basically derivates to be displayed.

Show outdated Hide outdated src/SCRIPTS/BF/HORUS/vtx.lua Outdated
@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 21, 2017

Member

The good news:

VTX control works with this pull request (and without it) for flight controllers with a VTX6705!

The bad news:

There's a syntax error. See comment.

Member

mikeller commented Nov 21, 2017

The good news:

VTX control works with this pull request (and without it) for flight controllers with a VTX6705!

The bad news:

There's a syntax error. See comment.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 21, 2017

Contributor

I'll fix that syntax error and address the other issues. Also, should be able to show the RTC6705 device type.

Contributor

ethomas997 commented Nov 21, 2017

I'll fix that syntax error and address the other issues. Also, should be able to show the RTC6705 device type.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 21, 2017

Member

That'd be great. All I get now is a lousy number (you can probably hide pit mode as well if it's an RTC6705):

img_20171121_234716

Member

mikeller commented Nov 21, 2017

That'd be great. All I get now is a lousy number (you can probably hide pit mode as well if it's an RTC6705):

img_20171121_234716

Made vtx freq editable
Can step through available frequencies for bands / channels.

If Cleanflight/Betaflight has 'SetFreqByMHzMsp' support then any
frequency in MHz can be set (via band 'U').
@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 23, 2017

Contributor

I've implemented fixes; should be in good shape now:

Fixed syntax error in BF/HORUS/vtx.lua
Put in proper 'self.' scoping prefixes
Added 'RTC6705' to VTX Dev table
Added RTC6705 entry to VTX power table
Don't display Pit field if device is RTC6705

Contributor

ethomas997 commented Nov 23, 2017

I've implemented fixes; should be in good shape now:

Fixed syntax error in BF/HORUS/vtx.lua
Put in proper 'self.' scoping prefixes
Added 'RTC6705' to VTX Dev table
Added RTC6705 entry to VTX power table
Don't display Pit field if device is RTC6705

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 23, 2017

Member

This looks a lot better now, thanks @ethomas997. The one thing that I am slightly concerned is that this adds quite a lot of code just to implement a relatively minor feature.

We are already getting problem reports (#97 (comment)) from users trying to use the betaflight-tx-lua scripts together with other scripts, and this will make it worse.

What's your thoughts on this, @raphaelcoeffic, @codecae?

Member

mikeller commented Nov 23, 2017

This looks a lot better now, thanks @ethomas997. The one thing that I am slightly concerned is that this adds quite a lot of code just to implement a relatively minor feature.

We are already getting problem reports (#97 (comment)) from users trying to use the betaflight-tx-lua scripts together with other scripts, and this will make it worse.

What's your thoughts on this, @raphaelcoeffic, @codecae?

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 23, 2017

Contributor

@mikeller: One significant space savings we could get right away would be to combine the 'vtx.lua' scripts for the three transmitter types. The files are duplicates except for the 'text' and 'fields' entries. If we break those out, there can be a single 'vtx.lua' script.

Contributor

ethomas997 commented Nov 23, 2017

@mikeller: One significant space savings we could get right away would be to combine the 'vtx.lua' scripts for the three transmitter types. The files are duplicates except for the 'text' and 'fields' entries. If we break those out, there can be a single 'vtx.lua' script.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 23, 2017

Member

@ethomas997: That is a good point. We should probably do that before this is merged.

Member

mikeller commented Nov 23, 2017

@ethomas997: That is a good point. We should probably do that before this is merged.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 23, 2017

Contributor

@mikeller: I was thinking we should it after this one is merged, as a separate PR, to better track the changes.

Contributor

ethomas997 commented Nov 23, 2017

@mikeller: I was thinking we should it after this one is merged, as a separate PR, to better track the changes.

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 23, 2017

Member

@ethomas997: Definitely in a separate pull request. I was thinking before, to avoid getting bug reports because of running out of memory, but if the consolidation is happening reasonably soon after this has been merged that will be fine as well.

Member

mikeller commented Nov 23, 2017

@ethomas997: Definitely in a separate pull request. I was thinking before, to avoid getting bug reports because of running out of memory, but if the consolidation is happening reasonably soon after this has been merged that will be fine as well.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Nov 23, 2017

Contributor

@mikeller I can make a point of doing the consolidation soon.

Contributor

ethomas997 commented Nov 23, 2017

@mikeller I can make a point of doing the consolidation soon.

@codecae

This comment has been minimized.

Show comment
Hide comment
@codecae

codecae Nov 23, 2017

Member

I don't think that the space savings will make much of a difference regarding memory consumption. Every time the screen is changed, garbage is collected and a new page object is loaded in place of the previous. Pulling common methods from another file probably won't help this very much, as the result would be the same. It would certainly help simplify maintenance of the code, though.

Member

codecae commented Nov 23, 2017

I don't think that the space savings will make much of a difference regarding memory consumption. Every time the screen is changed, garbage is collected and a new page object is loaded in place of the previous. Pulling common methods from another file probably won't help this very much, as the result would be the same. It would certainly help simplify maintenance of the code, though.

@codecae

This comment has been minimized.

Show comment
Hide comment
@codecae

codecae Nov 23, 2017

Member

The best way to ensure we are making use of our very precious memory resource is to be a good steward of variables and trim things down to the bare essentials.

Member

codecae commented Nov 23, 2017

The best way to ensure we are making use of our very precious memory resource is to be a good steward of variables and trim things down to the bare essentials.

@raphaelcoeffic

This comment has been minimized.

Show comment
Hide comment
@raphaelcoeffic

raphaelcoeffic Nov 23, 2017

Member

Exactly. The second thing would be to see whether we could split screens to make them smaller in terms of memory (MSP replies need to become smaller).

Member

raphaelcoeffic commented Nov 23, 2017

Exactly. The second thing would be to see whether we could split screens to make them smaller in terms of memory (MSP replies need to become smaller).

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Nov 23, 2017

Member

@codecae: Sorry, my previous answer was given without studying the code in detail. I assumed that the files in question were all used on the same page, and not on separate pages. If they are separate pages, then the 'space savings' argument is largely void. But as you say, the cleanup aspect still persists.

Member

mikeller commented Nov 23, 2017

@codecae: Sorry, my previous answer was given without studying the code in detail. I assumed that the files in question were all used on the same page, and not on separate pages. If they are separate pages, then the 'space savings' argument is largely void. But as you say, the cleanup aspect still persists.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Dec 9, 2017

Contributor

So, can this PR be merged in?

Contributor

ethomas997 commented Dec 9, 2017

So, can this PR be merged in?

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Dec 10, 2017

Member

Sorry, it dropped off the radar. I am good with it, @codecae, @raphaelcoeffic, any objections?

Member

mikeller commented Dec 10, 2017

Sorry, it dropped off the radar. I am good with it, @codecae, @raphaelcoeffic, any objections?

@mikeller mikeller merged commit 86e20f6 into betaflight:master Dec 11, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Dec 30, 2017

Contributor

Could a new release be posted to include these changes?

Contributor

ethomas997 commented Dec 30, 2017

Could a new release be posted to include these changes?

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Dec 30, 2017

Member

Sorry we've been slacking off a bit. (Or rather spending time on other areas of Betaflight.)

Unfortunately now seems to be a bad time for a release of the scripts because of the memory shortage that 2.2.1 brought about - if we release now, the additional space requirements for the new features will likely push more users over the edge. I think the right course of action is to wait and see what happens to opentx/opentx#5579 (my hope would be for a bugfix release to improve the memory situation).

Again, sorry for not looking into this earlier.

Member

mikeller commented Dec 30, 2017

Sorry we've been slacking off a bit. (Or rather spending time on other areas of Betaflight.)

Unfortunately now seems to be a bad time for a release of the scripts because of the memory shortage that 2.2.1 brought about - if we release now, the additional space requirements for the new features will likely push more users over the edge. I think the right course of action is to wait and see what happens to opentx/opentx#5579 (my hope would be for a bugfix release to improve the memory situation).

Again, sorry for not looking into this earlier.

@ethomas997

This comment has been minimized.

Show comment
Hide comment
@ethomas997

ethomas997 Dec 31, 2017

Contributor

Understood. You might want to add a note to the main README about the issues with OpenTX 2.2.1

Contributor

ethomas997 commented Dec 31, 2017

Understood. You might want to add a note to the main README about the issues with OpenTX 2.2.1

@mikeller

This comment has been minimized.

Show comment
Hide comment
@mikeller

mikeller Dec 31, 2017

Member

Good idea, done.

Member

mikeller commented Dec 31, 2017

Good idea, done.

codecae added a commit to codecae/betaflight-tx-lua-scripts that referenced this pull request Mar 30, 2018

Revert "Merge pull request betaflight#92 from ethomas997/etVtxFreqEdi…
…tableRev"

This reverts commit 86e20f6, reversing
changes made to 6c3ded4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment