Skip to content
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

Add iFlight Commando8 #1902

Closed
wants to merge 32 commits into from
Closed

Add iFlight Commando8 #1902

wants to merge 32 commits into from

Conversation

XING-IF
Copy link
Contributor

@XING-IF XING-IF commented Apr 26, 2022

Add iFlight Commando8 ELRS radio.
Based on the OpenTX PR #8905 with fixes.
opentx/opentx#8905

https://shop.iflight-rc.com/Commando-8-Radio-Transmitter-Pro1696

image

XING-IF added 2 commits April 26, 2022 15:04
Add iFlight Commando8 ELRS radio
@XING-IF XING-IF marked this pull request as ready for review April 26, 2022 08:16
Copy link
Member

@pfeerick pfeerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking a lot better than the original code I saw a few weeks ago :)

Because of the recent changes to the build system (#1736) the scripts in /tools will need updating.

companion/src/simulation/simulatorwidget.cpp Outdated Show resolved Hide resolved
@XING-IF
Copy link
Contributor Author

XING-IF commented Apr 26, 2022

This is looking a lot better than the original code I saw a few weeks ago :)

Because of the recent changes to the build system (#1736) the scripts in /tools will need updating.

ok, we'll check. thanks

.github/workflows/actions.yml Outdated Show resolved Hide resolved
.github/workflows/actions.yml Show resolved Hide resolved
radio/src/gui/128x64/model_setup.cpp Outdated Show resolved Hide resolved
radio/src/gui/128x64/model_setup.cpp Outdated Show resolved Hide resolved
radio/src/targets/taranis/CMakeLists.txt Outdated Show resolved Hide resolved
radio/src/targets/taranis/CMakeLists.txt Show resolved Hide resolved
radio/src/targets/taranis/board.cpp Outdated Show resolved Hide resolved
radio/src/targets/taranis/board.h Outdated Show resolved Hide resolved
tools/build-companion-nightly.sh Outdated Show resolved Hide resolved
@XING-IF
Copy link
Contributor Author

XING-IF commented Apr 28, 2022

All suggestions have been implemented and code should be fixed. We kindly ask for a re-review or further suggestions! Thanks guys

@pfeerick
Copy link
Member

This is looking starting to look pretty good now. No more breaking of other builds and code is starting to look reasonably clean :)

@raphaelcoeffic What needs to be done to kick the FreeRTOS change, as I don't think that should be there?

@XING-IF
Copy link
Contributor Author

XING-IF commented Apr 29, 2022

seems like all checks passed, no broken builds neither

XING-IF added 4 commits April 29, 2022 10:47
Add iFlight Commando8 ELRS radio
several code fixes after suggestions in PR
@raphaelcoeffic
Copy link
Member

This is looking starting to look pretty good now. No more breaking of other builds and code is starting to look reasonably clean :)

@raphaelcoeffic What needs to be done to kick the FreeRTOS change, as I don't think that should be there?

Looks like this slipped in: I just rebased on main and edited the culprit to remove the FreeRTOS change.

@pfeerick
Copy link
Member

Thanks! :) So, back to review mode...

ok, Windows simulator is kinda funky...
image

I'm not that familiar with the Companion/QT side of things, but it looks like the image for each of the sections (top, bottom, left and right side) are expected to be the full size of that section, not part-of, hence the smaller images repeating.

@pfeerick
Copy link
Member

@elecpower As the Companion whisperer, I defer to you for the Companion side of this :)

@pfeerick pfeerick requested a review from elecpower April 29, 2022 09:39
@elecpower
Copy link
Collaborator

Compiled branch and after a very quick check there are basic issues. So for starters Companion requires updated/checked:

  • board capabilities
  • firmware capabilities
  • isAvailable functions
  • radio profile default module
  • radio hardware settings
  • radio to radio conversions
  • reading opentx bin format
  • write yaml format
  • simulator graphic as per @pfeerick above
  • etc

@raphaelcoeffic
Copy link
Member

@elecpower side note: there is no OpenTx binary format here. The radio is basically unsupported on OpenTx side.

@XING-IF
Copy link
Contributor Author

XING-IF commented Apr 29, 2022

@elecpower side note: there is no OpenTx binary format here. The radio is basically unsupported on OpenTx side.

The OpenTX PR would be opentx/opentx#8905 but is not yet through.

@deadbytefpv
Copy link

Switch Config doesn't seem to stick.
SA to SD goes back into "None" after each power-cycle.
Radio.YML have these set up though.

@pfeerick
Copy link
Member

Mine arrived today, and I just flashed the latest PR build onto it. I initially got the 'None' setting for the switches (which I'm not so concerned about yet - will have to see what happens on a clean config - as it should be defaulting to correct setting if invalid), and then after setting them, it has stuck for several reboot cycles. Are you changing pages or exiting the radio setup menu before powering off the radio after setting them up?

@deadbytefpv
Copy link

deadbytefpv commented May 11, 2022

Mine arrived today, and I just flashed the latest PR build onto it. I initially got the 'None' setting for the switches (which I'm not so concerned about yet - will have to see what happens on a clean config - as it should be defaulting to correct setting if invalid), and then after setting them, it has stuck for several reboot cycles. Are you changing pages or exiting the radio setup menu before powering off the radio after setting them up?

Yes. I've actually kept the radio turned on quite a while before turning it off to see if the settings will stick..
I've also tested with a clean sd card to perhaps refresh the configs, but it still happened (settings went back to None after a reboot).
Thanks for checking things out!

@XING-IF
Copy link
Contributor Author

XING-IF commented May 11, 2022

@pfeerick @deadbytefpv I just checked with iFlight. The radio was obviously shipped with a custom build from their drives which is different to the code of this PR. The firmware version is misleading and says "Built by EdgeTX" which should actually say "Built by iFlight" as of this point the code was not merged yet. There were some fixes on their drive which have not been provided for this PR. I'm just trying to get some answers who fu**ed up again...

@pfeerick
Copy link
Member

pfeerick commented May 11, 2022

As far as the build it was shipped with, it seemed OK as far as the switches, but I didn't really try anything with it as I expected it to be out of date. Certainly need to liven someone up if they're not at least using the current state of this PR as the shipping build 😅

@XING-IF
Copy link
Contributor Author

XING-IF commented May 11, 2022

As far as the build it was shipped with, it seemed OK as far as the switches, but I didn't really try anything with it as I expected it to be out of date. Certainly need to liven someone ul if they're not at least using the current state of this PR as the shipping build 😅

I'm already releasing my anger and making sure this is a learned lesson. Mistakes can happen but should actually be found where they are made!

@deadbytefpv
Copy link

deadbytefpv commented May 11, 2022

I had a little bit more play...
Added a second model from scratch.
Reconfigured the switches and also setup my mixes with those switches..
Also did the mixes on the first model (that came with the radio).
Let it sit a bit.. some 3 minutes or so.
Turned it off and didn't touched the radio for a while (turned off).

Just turned it On a moment ago and the switches are there. Didn't reset to None. On both models.
I've since deleted the second model. Switched to the first original model. I've turned the radio off, and powered it back on, and the switches seems to be intact.
I'll have some more play later and see if switch config sticks.
Will report here if there's any change.
Thanks!

EDIT: The issue has come back. Not sure what triggered it. But cannot "fix" it again with the steps outlined above.

@raphaelcoeffic
Copy link
Member

Obsoleted by #1982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants