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

Fix STM32 CAN reset to not lose context #4932

Merged
merged 6 commits into from
Sep 20, 2017
Merged

Conversation

LMESTM
Copy link
Contributor

@LMESTM 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 mentioned this pull request Aug 18, 2017
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 18, 2017

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

@tommikas
Copy link
Contributor

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

@tommikas
Copy link
Contributor

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

@@ -137,8 +137,9 @@ struct pwmout_s {
};

struct can_s {
Copy link
Member

Choose a reason for hiding this comment

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

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

@adustm
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

0xc0170 commented Aug 21, 2017

retest uvisor

@adbridge
Copy link
Contributor

@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
Copy link
Contributor Author

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.

@studavekar
Copy link
Contributor

/morph test

@LMESTM
Copy link
Contributor Author

LMESTM commented Sep 4, 2017

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

@0xc0170
Copy link
Contributor

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

@LMESTM
Copy link
Contributor Author

LMESTM commented Sep 4, 2017

@0xc0170 thx !

@mbed-bot
Copy link

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
Copy link
Contributor

0xc0170 commented Sep 5, 2017

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

This will ease up further changes to the structure.
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.
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.
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.
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.
In case of F2 devices without CAN support.
@LMESTM
Copy link
Contributor Author

LMESTM commented Sep 5, 2017

@0xc0170 rebased now

@LMESTM LMESTM mentioned this pull request Sep 12, 2017
@Nodraak
Copy link
Contributor

Nodraak commented Sep 12, 2017

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

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 20, 2017

/morph test

@mbed-bot
Copy link

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1342

All builds and test passed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants