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

Fix STM32 CAN reset to not lose context #4932

Merged
merged 6 commits into from Sep 20, 2017

Conversation

Projects
None yet
9 participants
@LMESTM
Contributor

LMESTM commented Aug 18, 2017

Description

This pull request is a FIX to #4396
Resolves #4396
Up to now, can_reset for STM32 was not restoring the registers state as they were before reset,
which seems to be expected from application side.

Status

READY

Tests

Tessted by adding a call to can_reset() in the for loop of MBED_A28 CAN loopback test

Test ID Test Description Target Toolchain Result
MBED_A28 CAN loopback test NUCLEO_F446RE ARM OK
MBED_A28 CAN loopback test NUCLEO_F103RB ARM OK
MBED_A28 CAN loopback test NUCLEO_F091RC ARM OK
MBED_A28 CAN loopback test NUCLEO_F207ZG ARM OK
MBED_A28 CAN loopback test NUCLEO_L476RG ARM OK
MBED_A28 CAN loopback test NUCLEO_F303ZE ARM OK
MBED_A28 CAN loopback test NUCLEO_F767ZI ARM OK

@LMESTM LMESTM referenced this pull request Aug 18, 2017

Closed

STM32: CAN Reset() #4396

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 18, 2017

@tommikas @marhil01 @miklis Raas not accesible ? Please look at the failure

@tommikas

This comment has been minimized.

Contributor

tommikas commented Aug 18, 2017

@0xc0170 Thanks for the heads up. Looking into it.

@tommikas

This comment has been minimized.

Contributor

tommikas commented Aug 18, 2017

@0xc0170 The RaaS had become unresponsive. Moved to a different instance and builds are running again. I restarted the affected jobs.

targets/TARGET_STM/TARGET_STM32F2/objects.h Outdated
@@ -137,8 +137,9 @@ struct pwmout_s {
};
struct can_s {

This comment has been minimized.

@adustm

adustm Aug 21, 2017

Member

Hello Laurent,
Could you add #if DEVICE_CAN (in case of future STM32F2 device that does not support CAN) ?

@adustm

This comment has been minimized.

Member

adustm commented Aug 21, 2017

Hello Laurent,
I think it's ok, wrt what we discussed last week.
Let's see if it fits the need from @chrissnow

Don't you want to commit the modified MBED_A28 test ?

Kind regards
Armelle

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Aug 21, 2017

Thanks @adustm
I've added the #ifdef DEVICE_CAN

Don't you want to commit the modified MBED_A28 test ?

This makes the test a different one. So that would mean adding a new test in the unsupported folder.
I'd prefer to have CAN tested in MBED5 list of tests.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 21, 2017

retest uvisor

@adbridge

This comment has been minimized.

Contributor

adbridge commented Aug 24, 2017

@LMESTM

This makes the test a different one. So that would mean adding a new test in the unsupported folder.
I'd prefer to have CAN tested in MBED5 list of tests.

Does this require more work then ?

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Aug 24, 2017

@adbridge The PR does fix the reported bug, so I think it is ready for merge
I just don't plan to add a new MBED2 test as they're located in "unsupported" folder. If MBED2 tests are clearly in the plans for the future and moved to a supported folder, then I may create a new test.

@adbridge adbridge added needs: CI and removed needs: review labels Aug 24, 2017

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Sep 4, 2017

/morph test

@adustm

adustm approved these changes Sep 4, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

@0xc0170 @adbridge - any comments on this PR ? or can we move to next step ?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 4, 2017

@0xc0170 @adbridge - any comments on this PR ? or can we move to next step ?

Let me review again, but as I recall this was just waiting for CI

@0xc0170

0xc0170 approved these changes Sep 4, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 4, 2017

@0xc0170 thx !

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 5, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1203

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 5, 2017

@LMESTM Can you please resolve the conflict caused by one of the integrations yesterday?

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 5, 2017

LMESTM added some commits Aug 17, 2017

STM32: move can_s definition to common_objects.h
This will ease up further changes to the structure.
STM32: CAN: restore registers after can_reset
After reset the MCR register content needs to be restored so we're
introducing the can_registers_init function to be called at the first
init stage, but also after reset. We also store the can frequency to
go through the initialisation phase again.
STM32: Define CAN handle as part of can object
Instead of a static object, this will make driver
instantiation more robust and allow to re-use init
configuration on a need basis.

The CANName struct member is actually the CAN registers base address,
which is now available in the CanHandle.Instance field, so we don't need
CANName anymore.
STM32: CAN: do not overwite BTR register when setting frequency
BTR register has other bits than the ones calculated and set through
the can_speed function, so let's take care to only write to the
right registers.
STM32: CAN: store the mode in object context
In order to apply the same mode in case of reset, we store the current
requested mode in the HAL structure.

To make storage in a single place, we also change can_monitor to call
can_mode function as they actually acting on same registers.
STM32: F2: put can_s struct under DEVICE_CAN option
In case of F2 devices without CAN support.

@LMESTM LMESTM force-pushed the LMESTM:can_reset branch to 5997811 Sep 5, 2017

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Sep 5, 2017

@0xc0170 rebased now

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Sep 5, 2017

@LMESTM LMESTM referenced this pull request Sep 12, 2017

Closed

STM32 CAN bus #5059

@Nodraak

This comment has been minimized.

Contributor

Nodraak commented Sep 12, 2017

Ping @0xc0170, CI is successfull, I think you can merge :)

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 20, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1342

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 20, 2017

@theotherjimmy theotherjimmy merged commit a2cdb10 into ARMmbed:master Sep 20, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment