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

Make it easier to include all baro drivers to target #9394

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Conversation

mmosca
Copy link
Collaborator

@mmosca mmosca commented Oct 23, 2023

Some fc don't include a baro, in that case we should include all drivers in the target, so the user has flexibility in choseing an external baro, without having to create a custom target.

Downside is firmware size for f411/f722 devices.

Some fc don't include a baro, in that case we should include all drivers
in the target, so the user has flexibility in choseing an external baro,
without having to create a custom target.

Downside is firmware size for f411/f722 devices.
@mmosca mmosca added this to the 7.0 milestone Oct 23, 2023
@MrD-RC
Copy link
Collaborator

MrD-RC commented Oct 23, 2023

Could this be something that's disabled on targets with flash < 512kb? If the space is an issue.

@mmosca
Copy link
Collaborator Author

mmosca commented Oct 23, 2023

Could this be something that's disabled on targets with flash < 512kb? If the space is an issue.

This is mostly for fcs that don't have an onboard baro, as we have no idea what baro the users will try to use.

It currently fits in 512kb targets, and needs to be added manually to the target anyway. I don't plan to do a bulk change of targets, as I have no idea which targets have an onboard baro or not.

Having all these drivers does increase flash usage by about 1%, when comparing to no baro drivers.

@mmosca
Copy link
Collaborator Author

mmosca commented Oct 23, 2023

And just for the record, yes, it is possible to add #if (MCU_FLASH_SIZE <= 512) check to limit drivers to "most common", if we know what they are.

@Jetrell
Copy link

Jetrell commented Oct 23, 2023

Having all these drivers does increase flash usage by about 1%, when comparing to no baro drivers.

1% is more than 1/3 (497Kb) of the currently available flash still remaining on 512Kb targets..
No offense, it would be a shame to waste that flash on a fringe feature. When only a small user group will take advantage of it.

@mmosca
Copy link
Collaborator Author

mmosca commented Oct 23, 2023

It is a balance on user friendliness and flash space. We have no way of knowing which baro they would use and a lot of users are not really that keen on building custom targets.
I do agree that flash is at a premium on these fc, but there is currently free space for these drivers and it is easy to change in 1 place if we ever decide to limit baro drivers on those fcs based on popularity.

@mmosca
Copy link
Collaborator Author

mmosca commented Oct 23, 2023

For reference:
Before change (3 random baro drivers)

[100%] Linking C executable ../../../../bin/IFLIGHTF7_TWING.elf
Memory region         Used Size  Region Size  %age Used
        ITCM_RAM:        8648 B        16 KB     52.78%
      ITCM_FLASH:          0 GB        16 KB      0.00%
ITCM_FLASH_CONFIG:          0 GB        16 KB      0.00%
     ITCM_FLASH1:          0 GB       480 KB      0.00%
           FLASH:         848 B        16 KB      5.18%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      436897 B       480 KB     88.89%
             TCM:       18228 B        64 KB     27.81%
             RAM:       82496 B       192 KB     41.96%
       MEMORY_B1:          0 GB         0 GB

After change:

[100%] Linking C executable ../../../../bin/IFLIGHTF7_TWING.elf
Memory region         Used Size  Region Size  %age Used
        ITCM_RAM:        8616 B        16 KB     52.59%
      ITCM_FLASH:          0 GB        16 KB      0.00%
ITCM_FLASH_CONFIG:          0 GB        16 KB      0.00%
     ITCM_FLASH1:          0 GB       480 KB      0.00%
           FLASH:         908 B        16 KB      5.54%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      441293 B       480 KB     89.78%
             TCM:       18232 B        64 KB     27.82%
             RAM:       82808 B       192 KB     42.12%
       MEMORY_B1:          0 GB         0 GB

@YI-BOYANG
Copy link
Contributor

I am in favor of this approach, because GEPRC comes out with some GPS modules integrated with MS5611, DPS310 barometers, which are easy to connect to the FC as external barometers. However, the user's FC may not contain the drivers for these two barometers, which may cause some confusion for the user.

If all the barometers could be included in the firmware, the situation might be solved, provided the chip has enough flash memory.
image

@DzikuVx DzikuVx merged commit cb229b6 into master Oct 24, 2023
14 checks passed
@DzikuVx DzikuVx deleted the mmosca-baro-all branch October 24, 2023 17:32
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