Integrate the CAN.cpp file into the thingset Zephyr module

[luizvilla]

Hello

Context of the issue
We are using your can.cpp api together with the Thingset Zephyr module. This means we have two Zephyr modules linked to thingset: one is the thingset module and the other is the can.cpp together with the datanodes.

Description of the issue

We have to integrate the can.cpp file into our code which is actually useful for anyone using the thingset Zephyr module with can. There's very little code that is specific to our hardware (like the standby pin). We think this code should be integrated into your thingset module.

Cheers
Luiz

[martinjaeger]

That's a good point and applies not only to the CAN part, but also to the serial interface.

I've got quite a bit of code duplication between the BMS firmware and the Charge Controller firmware because they basically use the same can.cpp and serial.cpp files.

Now that the ThingSet library works as a proper Zephyr module, this may be done with the help of some Kconfig defines.

The interface to the application would be to provide the data_objects array and make the ThingSet module aware of available board hardware. I'll think about some approaches a bit more... suggestions are welcome!

[cfoucher-laas]

Hello Martin,
Here is raw suggestion on-the-fly:

• Edit can.cpp to drop the hardware-specific part (related to standby mode)
• Place can.cpp in zephyr/src, and add it to CMakeFile.txt, but including it only if a specific KMake CONFIG is defined (e.g. CONFIG_THINGSET_CAN)
• If required, add an header file which could be included by the user-code (notably the data nodes part)
• The hardware-specific part should be delegated to an external driver (does Zephyr can support standby in any way?)

[b0661]

Can we hide away the CAN/ Serial/ ... specifics by an abstract communication port before moving to this?

My adhoc suggestion:

/**
 * @brief A ThingSet communication port.
 *
 * Runtime port structure (in ROM) per port instance.
 */
struct ts_port {

    int (*open)(const struct ts_port *port);

    int (*close)(const struct ts_port *port);

    /**
     * @brief Receive message on port.
     *
     * @param[in] port Port to receive at.
     * @param[in] msg Pointer to message buffer to be used to receive.
     * @param[in] callback_on_received If callback is NULL receive returns on
     *                the next message. If the callback is set receive
     *                immediatedly returns and the callback is called on the
     *                reception of the message. Beware even in this case the
     *                callback may be called before the receive function
     *                returns.
     * @param[in] timeout_ms maximum time to wait in milliseconds.
     */
    int (*receive)(const struct ts_port *port, struct net_buf *msg,
                   int (*callback_on_received)(const struct ts_port *port,
                                           const ts_device_id_t *device_id,
                                           struct net_buf *msg),
                   uint32_t timeout_ms);

    /**
     * @brief Transmit message on port.
     *
     * @param[in] port Port to send at.
     * @param[in] msg Pointer to message buffer to be send.
     * @param[in] device_id Device ID of device to send the message to.
     * @param[in] callback_on_sent If callback is NULL send returns on the
     *                         next message. If the callback is set send
     *                         immediatedly returns and the callback is called
     *                         after the transmission of the message. Beware
     *                         even in this case the callback may be called
     *                         before the send function returns.
     * @param[in] timeout_ms maximum time to wait in milliseconds.
     */
    int (*transmit)(const struct ts_port *port,
                    const ts_device_id_t *device_id,
                    struct net_buf *msg,
                    int (*callback_on_sent)(const struct ts_port *port,
                                            const ts_device_id_t *device_id,
                                            struct net_buf *msg),
                    uint32_t timeout_ms);
};

[martinjaeger]

• If required, add an header file which could be included by the user-code (notably the data nodes part)

Where would this header file live? In the application or in the module? The problem is that the data nodes are currently obtained from other application files via extern. Maybe we need to have a dedicated API that starts the CAN threads from the application and also passes the ThingSet context.

• The hardware-specific part should be delegated to an external driver (does Zephyr can support standby in any way?)

No, a standby pin is currently not supported by the Zephyr CAN driver. I talked to the CAN maintainer (alexanderwachter) some time ago if it would make sense to include it in the driver, but I don't remember our conclusion anymore...

[martinjaeger]

@b0661 I agree, it makes sense to finalize the port abstraction first and then implement the CAN and serial directly based on that interface.

You would also move the entire driver handling into this library, so that the implementation is Zephyr-specific, correct? So the ThingSet module could still be used outside Zephyr by calling the process function, but within the Zephyr environment also the lower layer parts (CAN, serial, SPI) would be provided ready to use.

@cfoucher-laas See here for related discussion about port abstraction: LibreSolar/esp32-edge-firmware#29

[cfoucher-laas]

• If required, add an header file which could be included by the user-code (notably the data nodes part)

Where would this header file live? In the application or in the module? The problem is that the data nodes are currently obtained from other application files via extern. Maybe we need to have a dedicated API that starts the CAN threads from the application and also passes the ThingSet context.

What I meant with that part is that the data nodes should be part of the user code, but may rely on a module header.
Indeed, if there is dependency to the data nodes in within the can.cpp part, this is an issue that can only be fixed by providing an API to register the data nodes structure to the module. Maybe another approach than using an array could help (multiple calls to a module function to register each node?)

[b0661]

You would also move the entire driver handling into this library, so that the implementation is Zephyr-specific, correct?

Yes, I would have the port abstraction and maybe some common port functions in generic TS and the specific port implementation in the Zephyr directory (probably under zepyhr/ports/..). Such other implementations, e.g. for FreeRTOS, may easily be added without interfering with each other. I wouldn´t use the term device driver as it is - at least in Zephyr - already used for something else.

[martinjaeger]

I wouldn´t use the term device driver as it is - at least in Zephyr - already used for something else.

Yeah I meant that we are using (handling) the existing Zephyr drivers in the port implementations. ThingSet port is a good term.

[b0661]

Added #22 "RFC: Add ThingSet communication support framework" as a prototype where can.cpp may be integrated as a communication port.

[luizvilla]

Hello everyone,

While integrating Serial.cpp to our code, we've realized that all the configuration of the serial and the can are actually on the KConfig of the Zephyr folder. It will also be necessary to migrate them to the KConfig of the module when they are to be integrated.
In our case we are separating the thingset module from the thingset "implementation" which is the can.cpp and serial.cpp. Both have their own KConfig setup separately.

Luiz

[b0661]

While integrating Serial.cpp to our code, we've realized that all the configuration of the serial and the can are actually on the KConfig of the Zephyr folder. It will also be necessary to migrate them to the KConfig of the module when they are to be integrated.

I can think of two scenarios

1. The ThingSet serial port uses one instance of Zephyr's serial driver
2. The ThingSet serial port is also a serial driver by it's own.

In case 1. the ThingSet module might just configure the Zephyr driver instance to use for the ThingSet serial port. All other configurations can be done for and by the Zephyr driver instance.

In case 2. you have to provide all configuration options within the ThingSet module.

Does this cover your use case?