#453 Declined
Repository
f5soh
Branch
LP-542_Auto_Enable_modules
Repository
librepilot
Branch
next

LP-542 Auto Enable modules - LP-173 Config battery import

Author
  1. Lalanne Laurent
Reviewers
Description
  • LP-542 Try starting TxPID, CameraStab and Autotune modules onchange.

  • LP-542 Save HwSettings first so module can be enabled before moduleSettings savings

  • LP-542 Allow TxPID as mandatory module - Battery as optional module

  • LP-173 Battery module is enabled but not ready to run: Init settings

  • LP-173 uav settings import: make sure to import HwSettings first

  • Issues 2 related issues

Comments (7)

  1. Vladimir Zidar

    I'm not sure how to put this right, but I have always liked the way with LP firmware how memory requirements are determined at boot time, and no additional memory allocations are done after that. There is actually only single place that what I have found that does allocate memory from heap after the system has booted, and that is, ironically at place where system is preparing for reboot.

    Now this PR breaks that fine behaviour, however I have no idea or solution to offer that would keep the boot time memory allocation and also allow modules to be started later.

    On the other hand, is it really necessary to start the modules on settings change, or would it be maybe better/less intrusive solution to somehow allow saving of uninitialised UAVOs as sent by GCS?

    1. Lalanne Laurent author

      Changes give a chance to modules for start when needed and store settings without lost something. The nice behavior you describe still required assuming the Boot alarm is always active for every HwSettings change and in fact need a reboot for normal / safe usage - arming / flight. If you prefer not starting module it can be easily done by only initialize moduleSettings UAVO with 'live' module enabled and prevent module starting if Boot alarm is triggered

  2. Alessio Morale

    Beside Vladimir's comment I think this breaks another basic "rule" of our firmware: System module has now some hardcoded relationships with several non core modules

    The other side effect that may happen in memory tight systems is that a simple save operation for a setting may cause random issues caused by running out of free heap.

    Another thing to take into account is that currently Module's initialize/start functions are not supposed to be called more than once.

    If the ultimate target is to be able to save all settings regardless of enabled modules we should discuss about other possible solutions like:

    • Enabling all settings objects regardless of their current usage
    • Auto initialize each settings when an update operation is requested for such object

    I have a strong preference on the first solution (as it also allows to load the content of such settings).

  3. Lalanne Laurent author

    Feel free to do the needed changes in a correct way and include the fixes introduced in this files: battery.c, autotune.c, txpid.c. For CC3D, i prefer the second solution.

    I will decline this PR

    1. Philippe Renon

      The critics are valid and your PR is valid too (at least because it is triggering an interesting discussion).

      Please keep this PR open for now until we come to an agreement.

      PS : Enabling all settings has the advantage of checking that it works. A user can (I guess) enable all settings and it should work. Currently we don't know, until it happens, if such a case works...

  4. Vladimir Zidar

    Enabling all settings objects is currently an issue with f1 only, but with simple fix in LP-432, it might even be okay to initialize them all. There should be a way to automatically init all objects marked as settings and I can look into it, but have no cc3d with me to verify the memory usage.

  5. Vladimir Zidar

    Anyway, I have tested the approach of automatically initializing all objects marked as settings, and that indeed works very well. F4 targets have no problems with ram usage of course, and F1 is good to go with LP-432 + removal of otherwise unused faultsettings uavo (??). The problem remains with F3, where ram usage increase does have significant impact, leaving quite less than without initializing all settings objects. I have tracked down the ram usage, and while the uavo itself takes slightly more memory than data field size itself, it is the telemetry module which subscribes to changes in (all) uavos that consumes most of the ram.