[CAN] Current implementation is hard to control during run-time

[jalinei]

Hi,

I've been through can.c and can.h today.

I do have some questions and enhancements proposals.

• live_reporting_enable is defined at the sdk level (in sdk.h) and encompass both live reporting and control reporting, this makes it hard to deactivate selectively control broadcast and live broadcast. What is the rational behind that choice ? In the current implementation I'm not sure how to activate / deactivate control broadcasting at run time.
  

  thingset-zephyr-sdk/src/can.c

  Line 386 in 4874076

  while (live_reporting_enable

• CONFIG_THINGSET_CAN_CONTROL_REPORTING_PERIOD is a good way of initializing the control period, but at the moment it cannot be changed at run time. https://github.com/ThingSet/thingset-zephyr-sdk/blob/48740769cea0d9666fc9132cd8923f9ce2c088f9/src/can.c#L421C41-L421C85 IMO would be nice to be able to set that parameter. Would it be better to add it to sdk.h or inside the struct thingset_can ?

• Why not place these live_reporting_enable and period related parameters in thingset_can ?

• IMO broadcasting (both live and control) should also trigger a PRE tx callback so the user can retrieve a measure to broadcast for instance. Is there a reason why it should not ?

• Open question : Have you figured out a simple way to determine who is the master in a multi node context ? I understood that the node_addr assignment is random, so if 0x01 disconnects how to determine simply who is the "smallest" address wise ?

I'm not sure I fully understood everything here, sorry if some are dumb questions 😄

[jalinei]

#ifdef CONFIG_THINGSET_SUBSET_LIVE_METRICS
bool live_reporting_enable = IS_ENABLED(CONFIG_THINGSET_REPORTING_LIVE_ENABLE_PRESET);
uint32_t live_reporting_period = CONFIG_THINGSET_REPORTING_LIVE_PERIOD_PRESET;
#endif

In sdk.c live_reporting_enable is only defined when live subset is enabled, while the variable is also used by the handler thingset_can_control_reporting_handler.

[jalinei]

thingset-zephyr-sdk/src/can.c

Line 399 in 4874076

frame.id = THINGSET_CAN_TYPE_SF_REPORT | THINGSET_CAN_PRIO_REPORT_LOW

Here I would expect to find THINGSET_CAN_PRIO_CONTROL_LOW instead

[martinjaeger]

Hey @jalinei !

Thanks for your enhancement ideas, which make a lot of sense to me. I'm travelling at the moment and only looking at the code on my mobile, so I'll give some more detailed feedback next week.