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.

Sign up for GitHub

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

RFC: Breaking API Change: drivers: can: rework support for manual bus-off recovery #69460

Merged
merged 2 commits into from
Mar 2, 2024
Merged

RFC: Breaking API Change: drivers: can: rework support for manual bus-off recovery #69460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e71546a
drivers: can: rework support for manual bus-off recovery
henrikbrixandersen Feb 14, 2024
9426740
docs: release: migration-guide: 3.7: list CAN bus-off recovery changes
henrikbrixandersen Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions doc/hardware/peripherals/can/shell.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@ The following :ref:`Kconfig <kconfig>` options enable additional subcommands and
* :kconfig:option:`CONFIG_CAN_STATS` enables printing of various statistics for the CAN controller
in the ``can show`` subcommand. This depends on :kconfig:option:`CONFIG_STATS` being enabled as
well.
* :kconfig:option:`CONFIG_CAN_AUTO_BUS_OFF_RECOVERY` enables the ``can recover`` subcommand when
disabled.
* :kconfig:option:`CONFIG_CAN_MANUAL_RECOVERY_MODE` enables the ``can recover`` subcommand.

For example, building the :ref:`hello_world` sample for the :ref:`frdm_k64f` with the CAN shell and
CAN statistics enabled:
Expand Down Expand Up @@ -253,8 +252,8 @@ details on the supported arguments.
Bus Recovery
************

The ``can recover`` subcommand can be used for initiating recovery from a CAN bus-off event as shown
below:
The ``can recover`` subcommand can be used for initiating manual recovery from a CAN bus-off event
as shown below:

.. code-block:: console

Expand All @@ -265,5 +264,5 @@ The subcommand accepts an optional bus recovery timeout in milliseconds. If no t
the command will wait indefinitely for the bus recovery to succeed.

.. note::
The ``recover`` subcommand is only available if
:kconfig:option:`CONFIG_CAN_AUTO_BUS_OFF_RECOVERY` is disabled.
The ``recover`` subcommand is only available if :kconfig:option:`CONFIG_CAN_MANUAL_RECOVERY_MODE`
is enabled.
18 changes: 18 additions & 0 deletions doc/releases/migration-guide-3.7.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,24 @@ Controller Area Network (CAN)
* ``phase-seg1-data``
* ``phase-seg1-data``

* Support for manual bus-off recovery was reworked:

* Automatic bus recovery will always be enabled upon driver initialization regardless of Kconfig
options. Since CAN controllers are initialized in "stopped" state, no unwanted bus-off recovery
will be started at this point.
* The Kconfig ``CONFIG_CAN_AUTO_BUS_OFF_RECOVERY`` was renamed (and inverted) to
:kconfig:option:`CONFIG_CAN_MANUAL_RECOVERY_MODE`, which is disabled by default. This Kconfig
option enables support for the :c:func:`can_recover()` API function and a new manual recovery mode
(see the next bullet).
* A new CAN controller operational mode :c:macro:`CAN_MODE_MANUAL_RECOVERY` was added. Support for
this is only enabled if :kconfig:option:`CONFIG_CAN_MANUAL_RECOVERY_MODE` is enabled. Having
this as a mode allows applications to inquire whether the CAN controller supports manual
recovery mode via the :c:func:`can_get_capabilities` API function. The application can then
either fail initialization or rely on automatic bus-off recovery. Having this as a mode
furthermore allows CAN controller drivers not supporting manual recovery mode to fail early in
:c:func:`can_set_mode` during application startup instead of failing when :c:func:`can_recover`
is called at a later point in time.

Display
=======

Expand Down
17 changes: 7 additions & 10 deletions drivers/can/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,26 +62,23 @@ config CAN_ACCEPT_RTR
level.

config CAN_FD_MODE
bool "CAN FD"
bool "CAN FD support"
help
Enable CAN FD support. Not all CAN controllers support CAN FD.

config CAN_MANUAL_RECOVERY_MODE
bool "Manual bus-off recovery support"
help
Enable support for manual (non-automatic) recovery from bus-off state. Not all CAN
controllers support manual recovery mode.

config CAN_RX_TIMESTAMP
bool "Receiving timestamps"
help
This option enables a timestamp value of the CAN free running timer.
The value is incremented every bit time and starts when the controller
is initialized. Not all CAN controllers support timestamps.

config CAN_AUTO_BUS_OFF_RECOVERY
bool "Automatic recovery from bus-off"
default y
help
This option enables the automatic bus-off recovery according to
ISO 11898-1 (recovery after 128 occurrences of 11 consecutive
recessive bits). When this option is enabled, the recovery API is not
available.

config CAN_QEMU_IFACE_NAME
string "SocketCAN interface name for QEMU"
default ""
Expand Down
4 changes: 2 additions & 2 deletions drivers/can/can_esp32_twai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,9 @@ const struct can_driver_api can_esp32_twai_driver_api = {
.set_state_change_callback = can_sja1000_set_state_change_callback,
.get_core_clock = can_esp32_twai_get_core_clock,
.get_max_filters = can_sja1000_get_max_filters,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
#ifdef CONFIG_CAN_MANUAL_RECOVERY_MODE
.recover = can_sja1000_recover,
#endif /* !CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */
#endif /* CONFIG_CAN_MANUAL_RECOVERY_MODE */
.timing_min = CAN_SJA1000_TIMING_MIN_INITIALIZER,
#ifdef CONFIG_SOC_SERIES_ESP32
.timing_max = CAN_SJA1000_TIMING_MAX_INITIALIZER,
Expand Down
4 changes: 2 additions & 2 deletions drivers/can/can_fake.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,9 @@ static const struct can_driver_api fake_can_driver_api = {
.add_rx_filter = fake_can_add_rx_filter,
.remove_rx_filter = fake_can_remove_rx_filter,
.get_state = fake_can_get_state,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
#ifdef CONFIG_CAN_MANUAL_RECOVERY_MODE
.recover = fake_can_recover,
#endif /* CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */
#endif /* CONFIG_CAN_MANUAL_RECOVERY_MODE */
.set_state_change_callback = fake_can_set_state_change_callback,
.get_core_clock = fake_can_get_core_clock,
.get_max_filters = fake_can_get_max_filters,
Expand Down
7 changes: 4 additions & 3 deletions drivers/can/can_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,15 +249,16 @@ static inline int z_vrfy_can_get_state(const struct device *dev, enum can_state
}
#include <syscalls/can_get_state_mrsh.c>

#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
#ifdef CONFIG_CAN_MANUAL_RECOVERY_MODE
static inline int z_vrfy_can_recover(const struct device *dev, k_timeout_t timeout)
{
K_OOPS(K_SYSCALL_DRIVER_CAN(dev, recover));
/* Optional API function */
K_OOPS(K_SYSCALL_OBJ(dev, K_OBJ_DRIVER_CAN));

return z_impl_can_recover(dev, timeout);
}
#include <syscalls/can_recover_mrsh.c>
#endif /* CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */
#endif /* CONFIG_CAN_MANUAL_RECOVERY_MODE */

#ifdef CONFIG_CAN_STATS

Expand Down
4 changes: 2 additions & 2 deletions drivers/can/can_kvaser_pci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ const struct can_driver_api can_kvaser_pci_driver_api = {
.set_state_change_callback = can_sja1000_set_state_change_callback,
.get_core_clock = can_kvaser_pci_get_core_clock,
.get_max_filters = can_sja1000_get_max_filters,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
#ifdef CONFIG_CAN_MANUAL_RECOVERY_MODE
.recover = can_sja1000_recover,
#endif /* !CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */
#endif /* CONFIG_CAN_MANUAL_RECOVERY_MODE */
.timing_min = CAN_SJA1000_TIMING_MIN_INITIALIZER,
.timing_max = CAN_SJA1000_TIMING_MAX_INITIALIZER,
};
Expand Down
18 changes: 0 additions & 18 deletions drivers/can/can_loopback.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,21 +340,6 @@ static int can_loopback_get_state(const struct device *dev, enum can_state *stat
return 0;
}

#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
static int can_loopback_recover(const struct device *dev, k_timeout_t timeout)
{
struct can_loopback_data *data = dev->data;

ARG_UNUSED(timeout);

if (!data->common.started) {
return -ENETDOWN;
}

return 0;
}
#endif /* CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */

static void can_loopback_set_state_change_callback(const struct device *dev,
can_state_change_callback_t cb,
void *user_data)
Expand Down Expand Up @@ -388,9 +373,6 @@ static const struct can_driver_api can_loopback_driver_api = {
.add_rx_filter = can_loopback_add_rx_filter,
.remove_rx_filter = can_loopback_remove_rx_filter,
.get_state = can_loopback_get_state,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
.recover = can_loopback_recover,
#endif
.set_state_change_callback = can_loopback_set_state_change_callback,
.get_core_clock = can_loopback_get_core_clock,
.get_max_filters = can_loopback_get_max_filters,
Expand Down
37 changes: 24 additions & 13 deletions drivers/can/can_mcan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,13 @@ int can_mcan_get_capabilities(const struct device *dev, can_mode_t *cap)

*cap = CAN_MODE_NORMAL | CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY;

#if CONFIG_CAN_FD_MODE
*cap |= CAN_MODE_FD;
#endif /* CONFIG_CAN_FD_MODE */
if (IS_ENABLED(CONFIG_CAN_MANUAL_RECOVERY_MODE)) {
*cap |= CAN_MODE_MANUAL_RECOVERY;
}

if (IS_ENABLED(CONFIG_CAN_FD_MODE)) {
*cap |= CAN_MODE_FD;
}

return 0;
}
Expand Down Expand Up @@ -350,22 +354,24 @@ int can_mcan_stop(const struct device *dev)

int can_mcan_set_mode(const struct device *dev, can_mode_t mode)
{
can_mode_t supported = CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY;
struct can_mcan_data *data = dev->data;
uint32_t cccr;
uint32_t test;
int err;

#ifdef CONFIG_CAN_FD_MODE
if ((mode & ~(CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY | CAN_MODE_FD)) != 0U) {
LOG_ERR("unsupported mode: 0x%08x", mode);
return -ENOTSUP;
if (IS_ENABLED(CONFIG_CAN_MANUAL_RECOVERY_MODE)) {
supported |= CAN_MODE_MANUAL_RECOVERY;
}
#else /* CONFIG_CAN_FD_MODE */
if ((mode & ~(CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY)) != 0U) {

if (IS_ENABLED(CONFIG_CAN_FD_MODE)) {
supported |= CAN_MODE_FD;
}
Comment on lines +357 to +369
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call can_mcan_get_capabilities here in order to avoid code duplication?

Copy link
Member

@manuargue manuargue Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it seems that all drivers can benefit from this and avoid issues like #69460 (comment) :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yes - I too noticed that pattern when converting the drivers. I didn't want to mix that change into this PR, but I'm preparing to move this generic check into the can_set_mode() implementation function itself.


if ((mode & ~(supported)) != 0U) {
LOG_ERR("unsupported mode: 0x%08x", mode);
return -ENOTSUP;
}
#endif /* !CONFIG_CAN_FD_MODE */

if (data->common.started) {
return -EBUSY;
Expand Down Expand Up @@ -462,7 +468,8 @@ static void can_mcan_state_change_handler(const struct device *dev)
}
}

if (IS_ENABLED(CONFIG_CAN_AUTO_BUS_OFF_RECOVERY)) {
if (!IS_ENABLED(CONFIG_CAN_MANUAL_RECOVERY_MODE) ||
(data->common.mode & CAN_MODE_MANUAL_RECOVERY) == 0U) {
/*
* Request leaving init mode, but do not take the lock (as we are in ISR
* context), nor wait for the result.
Expand Down Expand Up @@ -847,7 +854,7 @@ int can_mcan_get_state(const struct device *dev, enum can_state *state,
return 0;
}

#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
#ifdef CONFIG_CAN_MANUAL_RECOVERY_MODE
int can_mcan_recover(const struct device *dev, k_timeout_t timeout)
{
struct can_mcan_data *data = dev->data;
Expand All @@ -856,9 +863,13 @@ int can_mcan_recover(const struct device *dev, k_timeout_t timeout)
return -ENETDOWN;
}

if ((data->common.mode & CAN_MODE_MANUAL_RECOVERY) == 0U) {
return -ENOTSUP;
}

return can_mcan_leave_init_mode(dev, timeout);
}
#endif /* CONFIG_CAN_AUTO_BUS_OFF_RECOVERY */
#endif /* CONFIG_CAN_MANUAL_RECOVERY_MODE */

int can_mcan_send(const struct device *dev, const struct can_frame *frame, k_timeout_t timeout,
can_tx_callback_t callback, void *user_data)
Expand Down
18 changes: 0 additions & 18 deletions drivers/can/can_mcp2515.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,21 +789,6 @@ static void mcp2515_handle_errors(const struct device *dev)
}
}

#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
static int mcp2515_recover(const struct device *dev, k_timeout_t timeout)
{
struct mcp2515_data *dev_data = dev->data;

ARG_UNUSED(timeout);

if (!dev_data->common.started) {
return -ENETDOWN;
}

return -ENOTSUP;
}
#endif

static void mcp2515_handle_interrupts(const struct device *dev)
{
const struct mcp2515_config *dev_cfg = dev->config;
Expand Down Expand Up @@ -904,9 +889,6 @@ static const struct can_driver_api can_api_funcs = {
.add_rx_filter = mcp2515_add_rx_filter,
.remove_rx_filter = mcp2515_remove_rx_filter,
.get_state = mcp2515_get_state,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
.recover = mcp2515_recover,
#endif
.set_state_change_callback = mcp2515_set_state_change_callback,
.get_core_clock = mcp2515_get_core_clock,
.get_max_filters = mcp2515_get_max_filters,
Expand Down
18 changes: 0 additions & 18 deletions drivers/can/can_mcp251xfd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -740,21 +740,6 @@ static int mcp251xfd_get_max_filters(const struct device *dev, bool ide)
return CONFIG_CAN_MAX_FILTER;
}

#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
static int mcp251xfd_recover(const struct device *dev, k_timeout_t timeout)
{
struct mcp251xfd_data *dev_data = dev->data;

ARG_UNUSED(timeout);

if (!dev_data->common.started) {
return -ENETDOWN;
}

return -ENOTSUP;
}
#endif

static int mcp251xfd_handle_fifo_read(const struct device *dev, const struct mcp251xfd_fifo *fifo,
uint8_t fifo_type)
{
Expand Down Expand Up @@ -1646,9 +1631,6 @@ static const struct can_driver_api mcp251xfd_api_funcs = {
.send = mcp251xfd_send,
.add_rx_filter = mcp251xfd_add_rx_filter,
.remove_rx_filter = mcp251xfd_remove_rx_filter,
#ifndef CONFIG_CAN_AUTO_BUS_OFF_RECOVERY
.recover = mcp251xfd_recover,
#endif
.get_state = mcp251xfd_get_state,
.set_state_change_callback = mcp251xfd_set_state_change_callback,
.get_core_clock = mcp251xfd_get_core_clock,
Expand Down
Loading
Loading