UAVCAN broken in stable / master

[mhkabir]

The UAVCAN dynamic node ID allocation changes got into stable, which increases memory consumption to a breaking point, and also renders the system unusable.

This patch gets it back online and usable (for now)

diff --git a/src/modules/uavcan/uavcan_main.cpp b/src/modules/uavcan/uavcan_main.cpp
index 2f3e1d5..85f55f9 100644
--- a/src/modules/uavcan/uavcan_main.cpp
+++ b/src/modules/uavcan/uavcan_main.cpp
@@ -53,10 +53,6 @@
 #include <drivers/drv_pwm_output.h>

 #include "uavcan_main.hpp"
-#include <uavcan_posix/dynamic_node_id_server/file_event_tracer.hpp>
-#include <uavcan_posix/dynamic_node_id_server/file_storage_backend.hpp>
-
-#include <uavcan_posix/firmware_version_checker.hpp>

 //todo:The Inclusion of file_server_backend is killing
 // #include <sys/types.h> and leaving OK undefined
@@ -75,20 +71,12 @@
  * UavcanNode
  */
 UavcanNode *UavcanNode::_instance;
-uavcan::dynamic_node_id_server::CentralizedServer *UavcanNode::_server_instance;
-uavcan_posix::dynamic_node_id_server::FileEventTracer tracer;
-uavcan_posix::dynamic_node_id_server::FileStorageBackend storage_backend;
-uavcan_posix::FirmwareVersionChecker fw_version_checker;

 UavcanNode::UavcanNode(uavcan::ICanDriver &can_driver, uavcan::ISystemClock &system_clock) :
    CDev("uavcan", UAVCAN_DEVICE_PATH),
    _node(can_driver, system_clock),
    _node_mutex(),
-   _esc_controller(_node),
-   _fileserver_backend(_node),
-   _node_info_retriever(_node),
-   _fw_upgrade_trigger(_node, fw_version_checker),
-   _fw_server(_node, _fileserver_backend)
+   _esc_controller(_node)
 {
    _control_topics[0] = ORB_ID(actuator_controls_0);
    _control_topics[1] = ORB_ID(actuator_controls_1);
@@ -154,7 +142,6 @@ UavcanNode::~UavcanNode()
    perf_free(_perfcnt_node_spin_elapsed);
    perf_free(_perfcnt_esc_mixer_output_elapsed);
    perf_free(_perfcnt_esc_mixer_total_elapsed);
-   delete(_server_instance);

 }

@@ -305,74 +292,6 @@ int UavcanNode::init(uavcan::NodeID node_id)
        br = br->getSibling();
    }

-
-   /* Initialize the fw version checker.
-   * giving it it's path
-   */
-
-   ret = fw_version_checker.createFwPaths(UAVCAN_FIRMWARE_PATH);
-
-   if (ret < 0) {
-       return ret;
-   }
-
-
-   /* Start fw file server back */
-
-   ret = _fw_server.start();
-
-   if (ret < 0) {
-       return ret;
-   }
-
-   /* Initialize storage back end for the node allocator using UAVCAN_NODE_DB_PATH directory */
-
-   ret = storage_backend.init(UAVCAN_NODE_DB_PATH);
-
-   if (ret < 0) {
-       return ret;
-   }
-
-   /* Initialize trace in the UAVCAN_NODE_DB_PATH directory */
-
-   ret = tracer.init(UAVCAN_LOG_FILE);
-
-   if (ret < 0) {
-       return ret;
-   }
-
-   /* Create dynamic node id server for the Firmware updates directory */
-
-   _server_instance = new uavcan::dynamic_node_id_server::CentralizedServer(_node, storage_backend, tracer);
-
-   if (_server_instance == 0) {
-       return -ENOMEM;
-   }
-
-   /* Initialize the dynamic node id server  */
-   ret = _server_instance->init(_node.getNodeStatusProvider().getHardwareVersion().unique_id);
-
-   if (ret < 0) {
-       return ret;
-   }
-
-   /* Start node info retriever to fetch node info from new nodes */
-
-   ret = _node_info_retriever.start();
-
-   if (ret < 0) {
-       return ret;
-   }
-
-
-   /* Start the fw version checker   */
-
-   ret = _fw_upgrade_trigger.start(_node_info_retriever, fw_version_checker.getFirmwarePath());
-
-   if (ret < 0) {
-       return ret;
-   }
-
    /*  Start the Node   */

    return _node.start();
diff --git a/src/modules/uavcan/uavcan_main.hpp b/src/modules/uavcan/uavcan_main.hpp
index dc81fb5..1157843 100644
--- a/src/modules/uavcan/uavcan_main.hpp
+++ b/src/modules/uavcan/uavcan_main.hpp
@@ -48,13 +48,6 @@
 #include "sensors/sensor_bridge.hpp"


-#include <uavcan/protocol/dynamic_node_id_server/centralized.hpp>
-#include <uavcan/protocol/node_info_retriever.hpp>
-#include <uavcan_posix/basic_file_server_backend.hpp>
-#include <uavcan/protocol/firmware_update_trigger.hpp>
-#include <uavcan/protocol/file_server.hpp>
-
-
 /**
  * @file uavcan_main.hpp
  *
@@ -83,7 +76,7 @@ class UavcanNode : public device::CDev

    static constexpr unsigned PollTimeoutMs      = 10;

-   static constexpr unsigned MemPoolSize        = 10752; ///< Refer to the libuavcan manual to learn why
+   static constexpr unsigned MemPoolSize        = 7168; ///< Refer to the libuavcan manual to learn why

    /*
     * This memory is reserved for uavcan to use for queuing CAN frames.
@@ -147,7 +140,6 @@ private:
    unsigned        _output_count = 0;      ///< number of actuators currently available

    static UavcanNode   *_instance;         ///< singleton pointer
-   static uavcan::dynamic_node_id_server::CentralizedServer *_server_instance;              ///< server singleton pointer

    Node            _node;              ///< library instance
    pthread_mutex_t     _node_mutex;
@@ -155,11 +147,6 @@ private:
    UavcanEscController _esc_controller;


-   uavcan_posix::BasicFileSeverBackend _fileserver_backend;
-   uavcan::NodeInfoRetriever  _node_info_retriever;
-   uavcan::FirmwareUpdateTrigger  _fw_upgrade_trigger;
-   uavcan::BasicFileServer        _fw_server;
-
    List<IUavcanSensorBridge *> _sensor_bridges;        ///< List of active sensor bridges

    MixerGroup      *_mixers = nullptr;

[LorenzMeier]

@pavel-kirienko We will relatively soon switch from an agile development model in terms of user releases to the currently established more stable baseline. It would be great if we could look tomorrow into the RAM usage and stability issues here, as the merge window is closing. The current memory consumption of UAVCAN doesn't allow it to launch it as default module and we already need to de-feature the system (reduce e.g. buffer sizes for the logging system) to fit it when its running.

[pavel-kirienko]

@LorenzMeier I have already proposed a solution here OpenCyphal-Garage/libcyphal#33: make the core UAVCAN functionality enabled by default, whereas the dynamic node ID allocator and firmware upgrade master would be optional. I'm well aware of usability penalties, but so far this is the only solution that is radical enough.

Also, when my suggestions described here are implemented in the uavcan driver, the memory use will further drop by 2~3K (although that's not enough to be a solution).

look tomorrow into the RAM usage and stability issues here

Are you proposing to have a call?

@mhkabir Is it the original patch I sent you two days ago?

[davids5]

@LorenzMeier , @pavel-kirienko

What time UTC should we have a call to define the short term and long term goals that will best align with the new release strategy?

[davids5]

PR #2404 is pushsed for short term conditional removal. Working on long term solution next

[davids5]

@pavel-kirienko , @LorenzMeier

Results of UAVCAN_MAX_NETWORK_SIZE_HINT testing

No change in elf sections sizes, but reducing the allocation from 64 to UAVCAN_MAX_NETWORK_SIZE_HINT=16 saved 5248 Bytes!

[LorenzMeier]

Ok, can we get that as PR on top of the current changes?

[LorenzMeier]

@pavel-kirienko this is what the MAVLink app is consuming - it routes all information of the system at 12K usage. In my view already pretty heavy, but it does missions / FTP / all system streams. I did already remove unnecessary stack sizes and buffers.

nsh> free
             total       used       free    largest
Mem:        225968     174352      51616      50352
nsh> mavlink start -d /dev/ttyS2
mavlink_if1: mode: 0, data rate: 2880 B/s on /dev/ttyS2 @ 57600B
nsh> mavlink_if1: Disabling hardware flow control
free
             total       used       free    largest
Mem:        225968     186080      39888      36256

Now when I start uavcan, I'm out of RAM:

free
             total       used       free    largest
Mem:        225968     186080      39888      36256
nsh> 
nsh> 
nsh> uavcan start
uavcan: Node ID 1, bitrate 1000000
uavcan: SW version vcs_commit: 0x959333d6
uavcan: sensor bridge 'gnss' init ok
uavcan: sensor bridge 'mag' init ok
uavcan: sensor bridge 'baro' init ok
nsh> free
             total       used       free    largest
Mem:        225968     210304      15664      10624

Note that the sdlog2 app will now fail to log, as it doesn't have enough RAM for its log buffer. So we really need to get UAVCAN closer to the 12-14K mark to fit.

[davids5]

@pavel-kirienko does

static constexpr unsigned MemPoolSize = 10752; ///< Refer to the libuavcan manual to learn why

Still make sense after all the changes you did?

[pavel-kirienko]

@davids5 I introduced some memory optimizations today, particualrly:

1. Removed the allocation table cache from the centralized server implementation, which saved 2144 bytes. The distributed server is still disgustingly huge though.
2. Fixed structure packing - it used to break things and increase code size because it was used inappropriately (so it could not be enabled), now it's organized properly (and it should be safe to enable it).
3. Rearranged some class fields to reduce padding. This change together with the previous one allowed to reduce block size to 48 bytes on ARM, which is a huge win (didn't test on any real hardware yet though).

This is what I suggest to add to the module defines:

-DUAVCAN_MEM_POOL_BLOCK_SIZE=48
-DUAVCAN_PACKED_BEGIN='_Pragma("pack(1)")' 
-DUAVCAN_PACKED_END='_Pragma("pack()")'

The pool size can be reduced for now:

static constexpr unsigned MemPoolSize = 64 * uavcan::MemPoolBlockSize;

Hopefully it should work.

@LorenzMeier @davids5 May I suggest to use -fpacked-enums to reduce general memory footprint? (not only libuavcan)

[LorenzMeier]

That sounds like good progress! Inline with what we discussed yesterday it would be great if you could also test this on the actual hardware. The main motivation is because it will help the protocol development to fit the embedded space.

[LorenzMeier]

@pavel-kirienko I will look into -fpacked-enums, but changes like this need to be vetted very carefully when done for the full system. Also because we probably need to apply it to the OS.

[pavel-kirienko]

@LorenzMeier Yep, I will set up a testing rig here. The packing turned out to hard fault the system so 48 byte blocks are still not acceptable, but I'll be working on it some more tomorrow and hopefully a way will be devised to get the memory consumption down to the 14K limit you set above.

[pavel-kirienko]

OK, I succeeded in reducing the pool block size to 48 bytes through some optimizations deep inside libuavcan. Current head of uavcan_size_investigation:

nsh> free
             total       used       free    largest
Mem:        223888      29360     194528     154944
nsh> uavcan start
<...>
nsh> free
             total       used       free    largest
Mem:        223888      44160     179728     154944

14800 bytes.

[LorenzMeier]

Awesome! @pavel-kirienko What is the testing state of this - is this ready for the stable branch?

[pavel-kirienko]

libuavcan is, but I'm not sure about the rest of the branch - let's hear from @davids5 who created it initially.

[pavel-kirienko]

UAVCAN firmware update logic takes 27k.

nsh> free
             total       used       free    largest
Mem:        224336      28256     196080     156608
nsh> uavcan start
<...>
nsh> free
             total       used       free    largest
Mem:        224336      41760     182576     156608
nsh> uavcan start fw
nsh> free
             total       used       free    largest
Mem:        224336      69152     155184     133184

@davids5 The hanging issue we discussed last night has been fixed via 42eb1d1.

[pavel-kirienko]

After some adjustments, the firmware update logic takes 26K (see nuttx_next_uavcan_multi_threaded).

[davids5]

Here are the results for nuttx_next_uavcan_multi_threaded 853b9dc

[davids5]

Here are the figures for the px4cannode were there was a 2064 byte savings!

[pavel-kirienko]

I wonder why do our results differ by 1k.

[davids5]

@pavel-kirienko

Could it be the 128 vs 64 magic number in

static constexpr unsigned MemPoolSize = 64 * uavcan::MemPoolBlockSize;

Or the fact I get a CCASERT failure with 48 and have to use 56?

[davids5]

Here are the results on master_uavcan_modular 500a347

You will note an oddity - what looks like a 592 byte mem leak after a stop, but a restart and stop of the fw thread returns to the original. - I will recheck the lifecycle management to see if I missed anything.

The main uavcan task when stopped look to leak 928 bytes. - think this may have been in the original master, and I will look into it also.

[LorenzMeier]

Ok. When you both think its ready for prime-time it would be great to have a PR against master. Because of the shallow testing I do not want to launch it by default, however, it should launch for all configs when the UAVCAN_ENABLED param is set to 1. We will later then default it to on. This will allow uavcan users to enable it from the GUI, but still shield the release from any side-effects we might have been missing.

[pavel-kirienko]

Okay. I shudder to ask, how much time do we have until the release?