-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Spektrum SRXL2 implementation #5791
Conversation
There are still some issues with this patch series (#5757 and #5791 patches applied to current dev branch):
|
60f3b5b
to
335876d
Compare
@stronnag just made a commit that should address this. Forgot to commit the source.mk file. Also fixed the strict prototype warnings |
@MiguelFAlvarez can you please rebase on top of development? This should cleanly rebase/merge now |
335876d
to
b0cbe99
Compare
@digitalentity I rebased on master because it looks like development was removed. Hope that's the correct one. Looks like all checks pass now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this PR still uses RTC6705 which we don't support anymore and doesn't account for FFPV 2.4 GHz VTX.
To avoid creating more dependencies, please don't use any VTX-specific tables for power or frequency. Use generic power indexes (but note that they are VTX specific and actual power/index mapping is determined at run-time to match capabilities of the actual hardware used).
What we could consider is to add a protocol-agnostic API to convert from mW of power to VTX power index so we can do all the conversions within the VTX driver and keep control code nice and simple.
src/main/drivers/vtx_common.h
Outdated
#define VTX_COMMON_BAND_RACE 5 | ||
|
||
// RTC6705 RF Power index 25 or 200 mW | ||
#define VTX_6705_POWER_25 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6705 is not supported on INAV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all of the vtx code with the exception of a few placeholders blocked by USE_SPEKTRUM_VTX_CONTROL
. If anything, will just make a pull request later on with support for this.
src/main/drivers/vtx_common.h
Outdated
#define VTX_6705_POWER_25 1 | ||
#define VTX_6705_POWER_200 2 | ||
|
||
// SmartAudio "---", 25, 200, 500, 800 mW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have unified frequency/power control now and number of power levels is dependent on the driver and actual VTX used. Hard coded values are not necessary.
src/main/fc/cli.c
Outdated
default: | ||
cliPrint("Not supported."); | ||
break; | ||
#if defined(USE_RX_FRSKY_SPI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
src/main/io/spektrum_vtx_control.c
Outdated
|
||
// RF Power Index translation tables. No generic power API available..... | ||
|
||
#ifdef USE_VTX_TRAMP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use VTX-protocol specific tables. Use unified power level API and note that power level is dependent on the driver and actual hardware connected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, otherwise LGTM
cfb7976
to
f08f9a0
Compare
The PR implementing SRXL2 (#5791) has not been merged into master and currently has a number of (trivial) conflicts that prevent it from being merged for 2.6. We have also not heard from the developer for a while. I've downloaded the 2.6.0 configurator and flashed the Hex file but then no SRXL2 in serial tab, do we know when this will happen as I may need to go back to 2.5 |
When we left off on this, it was ready to be merged and it was just a matter of waiting for 2.6 to start coming around. Didn't realize some conflicts had arisen since then, and nobody had tried reaching out about the conflicts. I'll take a look at this later today to get them corrected. |
Thank you, MiguelFAlvarez |
This consists of various files/changes brought over from betaflight with modifications to operate in the current state of inav. It also includes files/changes that were not a part of the betaflight SRXL2 merge, as the previous bidi srxl implementation was not yet implemented either, and SRXL2 has some dependencies on the Spektrum telemetry structuring from those files.
f08f9a0
to
5df85c2
Compare
@stronnag |
Thanks for fixing the merge issues. There is no "conflict monitor"; we all have to watch our own PRs prior to merge. Anyway, as this feature already has approval, I'm going to merge it. @MiguelFAlvarez, would you please also consider (separate PR), adding some basic documentation (To Rx.md / Telemetry.md as appropriate), e.g. any connection requirements (RX/TX pin(s)) and how to enable telemetry / telemetry data available; as you think appropriate. |
I had been checking on the PR every few days, but mainly just to make sure it ended up getting merged lol. Was unaware that merge conflicts would not notify me. Good to know for future requests that I'll have to pay special attention to that. And yeah I'll look into getting a quick doc added. Will probably throw in the same notes I had from my temporary releases. |
Thanks. |
Thanks I will flash the Hex files tomorrow. |
Thanks all for your contributions! I've attempted to use SRXL2 in iNav today, and I'm running into some issues getting it to work. Running the NEXT build, I'm left with all channels at I'm using a Spektrum SPM4650 connected to TX2 of a Matek F405-SE with a Spektrum DX6i controller. This configuration works perfectly in Betaflight, so hardware configuration is unlikely the cause. I built a version from HEAD with debug messages added back in and sent through the MSP connection. With that enabled, I see a loop every 250ms of I also tried out pulling the modified 3.5.0 branch of https://github.com/MiguelFAlvarez/inav and merging in support of the DPS310 barometer on my F405-SE. In this configuration, SRXL2 still did not work. I'm happy to make any changes y'all think would get this going - there's bad weather the next few days, so no reason to run Betaflight for now. :) |
Connect to RX2 my works fine |
@FrankPetrilli it looks like TX2 on that FC is used for smartport, and therefore will have a hardware inverter which breaks compatibility with SRXL2. Please try a different uart TX pin. |
Interesting, thanks all! I was able to get it to work by finding a hybrid approach. :) @tourerjim RX2 did not work - same results as TX2 Thanks! |
This PR includes support for Spektrum SRXL2. This is based off the betaflight PR betaflight/betaflight#8598 but there are a lot more changes here as some of the other spektrum features required did not yet exist in inav.
As opposed to the betaflight PR which uses the UART Idle interrupt, this code polls the idle flag in the rcFrameStatusFn as requested.
This PR also depends on #5757 which at this time has been approved, but not yet merged. Until it is merged, compile checks won't pass.