8000 Fixed swapped ifdefs where disabling Jeti XBUS disables Spektrum SRXL telemetry and vice versa by sebsx · Pull Request #6442 · betaflight/betaflight · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixed swapped ifdefs where disabling Jeti XBUS disables Spektrum SRXL telemetry and vice versa #6442

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

Merged

Conversation

sebsx
Copy link
@sebsx sebsx commented Jul 24, 2018

Probably a copy/paste error.

@sebsx sebsx changed the title Fixed swapped ifdefs where disabling Jeti XBUS disables SRXL and vice versa Fixed swapped ifdefs where disabling Jeti XBUS disables Spektrum SRXL telemetry and vice versa Jul 24, 2018
mikeller
mikeller previously approved these changes Jul 24, 2018
Copy link
Member
@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Good find @sebsx!

@@ -28,6 +28,10 @@
#define USBD_PRODUCT_STRING "CrazyBee F3 FR"
#endif

// clear up some flash space
#undef USE_PWM
Copy link
Member

Choose a reason for hiding this comment

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

  • the cleanup should be done in a separate pull request;
  • the list used for cleanup should be the same in all targets, or else managing flash space on F3 will be even more painful as it already is;
  • Removed more features from F3 to make the firmware fit into flash. #6444 already takes care of it (but happy to look into using the reductions suggested by you instead, if they are sufficient).

Copy link
Author

Choose a reason for hiding this comment

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

Noted. I reverted the last commit and will submit another PR with the same set of (ideally rarely used) features removed

Copy link
Member

Choose a reason for hiding this comment

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

Hold on, repurposing of #6444 already in progress.

@sebsx
Copy link
Author
sebsx commented Jul 24, 2018

@mikeller I added another commit to fix overruns on some F3 targets by disabling specific options like PWM input, MAVLINK telemetry and protocols like SUMH and XBUS. Not sure if it's the best approach as we should probably try to keep the same feature set for all the boards in the same family I suppose.

Regarding #6444, removing SRXL telemetry is probably not a good idea IMO since modern Spektrum receivers will be using it and moving back to Spektrum2048 means re-wiring the receiver to an RX pin instead of a TX pin (and losing telemetry in the process).

@AndersHoglund
Copy link
Contributor

Wow. Interesting find. How did this slip by inspections?

#undef USE_TELEMETRY_JETIEXBUS
#endif

#if !defined(USE_SERIALRX_JETIEXBUS)
#if !defined(USE_SERIALRX_SPEKTRUM)
#undef USE_TELEMETRY_SRXL
Copy link
Contributor

Choose a reason for hiding this comment

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

This #undef should be moved to the section below with other undefs depending on #ifndef USE_SERIALRX_SPEKTRUM

Copy link
Author

Choose a reason for hiding this comment

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

Good idea

@sebsx
Copy link
Author
sebsx commented Jul 24, 2018

Updated as per latest suggestions. Let me know if a squash is in order.

@mikeller
Copy link
Member

@sebsx: Yes please, a rebase is needed to get the flash space reductions in.

@sebsx sebsx force-pushed the fix-mixed-up-spektrum-and-jeti-ifdefs branch 2 times, most recently from 9506916 to 29446a6 Compare July 24, 2018 23:26
… versa

Consolidated disabled features when USE_SERIALRX_SPEKTRUM is disabled
@sebsx sebsx force-pushed the fix-mixed-up-spektrum-and-jeti-ifdefs branch from 29446a6 to 448a57c Compare July 24, 2018 23:27
@sebsx
Copy link
Author
sebsx commented Jul 24, 2018

Should be alright now.

@sebsx
Copy link
Author
sebsx commented Jul 25, 2018

@mikeller apparently we still have some boards overflowing

@mikeller
Copy link
Member

@sebsx: Yes we do. Good thing playing feature whack-a-mole is one of my favourite pasttimes. (It isn't.) Will look into it in a while.

@sebsx
Copy link
Author
sebsx commented Jul 25, 2018

@mikeller I noticed, but, in case you need a hand I don't mind experimenting a bit.

I think we should be able to disable on a board by board basis, for example CRAZYBEEF3FR has an integrated FrSky receiver so there's a slim chance someone hooks it up with a Spektrum or FlySky receiver. IN this case disabling a bunch of protocols should not be a problem. Same with other boards, disabling PWM where there are no PWM input pins, etc (I realise this should be done by the board's maintainer but I guess most older boards are abandoned by now).

What do you think?

@mikeller
Copy link
Member

@sebsx: I have taken these on board, and integrated them into the list of candidates to be disabled. SUMH is really superseded by SUMD, and I consider PWM to be obsolete as well.

@sebsx
Copy link
Author
sebsx commented Jul 25, 2018

@mikeller Cheers! 8000

@mikeller mikeller merged commit 74d883b into betaflight:master Jul 25, 2018
mikeller added a commit that referenced this pull request Jul 25, 2018
…defs

Fixed swapped ifdefs where disabling Jeti XBUS disables Spektrum SRXL telemetry and vice versa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0