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.

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

stm32/can: add option to enable deep-sleep per device #14911

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

vincent-d
Copy link
Member

Contribution description

Deep-sleep was based on using rx pin as external interrupt to be able to
wake up from stop mode. If rx pin cannot be used as interrupt or user
does not need to wake up from stop from the CAN, an option is now
present. If en_deep_sleep_wake_up is set to false, setting the device to
sleep simply unblocks stop mode. Otherwise the behavior is unchanged.

@vincent-d vincent-d added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: cpu Area: CPU/MCU ports labels Aug 31, 2020
@vincent-d vincent-d requested a review from aabadie August 31, 2020 14:29
@@ -100,6 +100,7 @@ typedef struct {
#ifndef CPU_FAM_STM32F1
gpio_af_t af; /**< Alternate pin function to use */
#endif
bool en_deep_sleep_wake_up; /**< Enable deep-sleep wake-up interrupt */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not extend the bit field below? (line 130)

Copy link
Member Author

Choose a reason for hiding this comment

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

The bitfield was using uint8_t as the base type and there were already eight bits in it. Also, the bits in the bit field correspond to a setting in the config register.

@aabadie
Copy link
Contributor

aabadie commented Sep 28, 2020

How can we test this ? I don't find any board that provides a default can configuration. The tests/candev application is also missing non native initialization code. This is out of the scope of this PR but would help if someone wants to try CAN on STM32.

@vincent-d
Copy link
Member Author

@aabadie tests/candev is improved in #14909 so it can be used more easily with non-native devices. tests/conn_can might also be a good candidate for testing.

Normally, the nucleo-f413zh provides a CAN configuration, but I must admit this is not straightforward, as a default can_params.h is shared for all stm32 CPUs. You would also need a transceiver.

@aabadie
Copy link
Contributor

aabadie commented Sep 28, 2020

So I guess the default can_params should be updated as well:

/** Default STM32 CAN devices config */
static const can_conf_t candev_conf[] = {

the nucleo-f413zh provides a CAN configuration

There's nothing related to CAN at this level.

@fjmolinas
Copy link
Contributor

So I guess the default can_params should be updated as well:

/** Default STM32 CAN devices config */
static const can_conf_t candev_conf[] = {

the nucleo-f413zh provides a CAN configuration

There's nothing related to CAN at this level.

@vincent-d this one looks good, do you mind adding the default config as well?

Vincent Dupont added 2 commits February 2, 2021 15:32
Deep-sleep was based on using rx pin as external interrupt to be able to
wake up from stop mode. If rx pin cannot be used as interrupt or user
does not need to wake up from stop from the CAN, an option is now
present. If en_deep_sleep_wake_up is set to false, setting the device to
sleep simply unblock stop mode. Otherwise the behavior is unchanged.
Add en_deep_sleep_wake_up = true in default candev_conf in can_params.h
@vincent-d vincent-d force-pushed the pr/can_stm32_deepsleep_opt branch from 5da9e09 to 2edf37e Compare February 2, 2021 14:40
@vincent-d
Copy link
Member Author

Added en_deep_sleep_wake_up  in default can configs and rebased.

@fjmolinas
Copy link
Contributor

All good, mind if I ask was what the use case for this is?

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK.

@vincent-d
Copy link
Member Author

All good, mind if I ask was what the use case for this is?

On stm32, EXTIs are a bit annoying...for instance, you cannot configure PA1 and PB1 as an external interrupt pin at the same time (there are 15 lines for EXTI, shared among ports).
We had such a conflict with the CAN RX pin on one of our CAN controllers. But on the other controller, we need to be able to wake up from stop mode, and the only way of doing it is to configure the RX pin as an external interrupt. So we needed a way to enable or disable the change of configuration of the RX pin.

@vincent-d vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 3, 2021
@benpicco benpicco merged commit d014f5e into RIOT-OS:master Feb 22, 2021
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants