-
Notifications
You must be signed in to change notification settings - Fork 17.2k
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
Remove coupling between AP_RCProtocols SRXL2 and DSM #23253
Remove coupling between AP_RCProtocols SRXL2 and DSM #23253
Conversation
aa7535b
to
5f47d96
Compare
b0f1d80
to
2309713
Compare
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 sure the define inclusion is correct
@@ -21,6 +21,14 @@ | |||
#include <AP_Vehicle/AP_Vehicle_Type.h> | |||
#include <AP_Logger/AP_Logger.h> | |||
|
|||
// for video TX configuration: | |||
#if AP_VIDEOTX_ENABLED |
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 don't think you can do this can you because the define is inside the video tx header
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 can get away with it because the header is transitively included via AP_RCProtocol.h
I've added an explicit include in this file.
.... that transitive include is a work of evil - the include at the end of AP_RCProtocol.h....
2309713
to
fb48043
Compare
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.
Change looks good to me, would just like to verify that nothing is broken as this is a code path that doesn't get a lot of use
I'm happy to buy some hardware to test with if you can point me at it. I'd rather not have to wait for that hardware to arrive, however :-) |
I can test - give me a few days |
fb48043
to
a52e0f9
Compare
this is called as a static method from DSM to SRXL2 which isn't good.
NFC, just moving from one cpp to another
a52e0f9
to
bb95033
Compare
So I did that :-) Any luck? |
Ping @andyp1per |
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.
Urg, I have realized I can't test this as I sold my more modern Spektrum TX and the older DX8 I have does not support VTX updates. So better just go in.
Thanks @andyp1per . Can you describe what hardware could be used to test this sort of thing? |
It needs a modern Spektrum TX. Since I basically maintain the Spektrum code I should probably put a proposal forward to the committee for one. I have DX8 - an up-to-date DX8 would be sufficient. |
Having the DSM backend calling into the SRXL2 backend isn't nice, so this fixes that.
Two commits, one changing namespaces the other moving the code into the correct files.