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

Bluepill: hse config fix #4755

Merged
merged 1 commit into from Jul 27, 2017

Conversation

Projects
None yet
7 participants
@LordGuilly
Contributor

LordGuilly commented Jul 13, 2017

Notes:

Description

fixed the BLUEPILL (and likely other F1XX devices) oscillator setup
Problem described in issue #4753

Status

READY

Migrations

NO

Todos

  • Tests
  • Documentation

Deploy notes

as said, it may impact other F1xx branches, but the HSI (fallback of the startup procedure) was doing 64Mhz), so systems may do faster than expected

Steps to test or reproduce

I started the mbed build app in a remote target with GDB, and printed SystemCoreClock

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 13, 2017

I don't have BLUEPILL target,
but I could reproduce the issue with another DISCO board (F429ZI) which supports also HSE_XTAL and not HSE_EXTC.
Proposed patch is solving the issue.

@0xc0170 0xc0170 added the needs: CI label Jul 13, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 13, 2017

Thanks for making the PR @LordGuilly. I really like the minimal diff.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 14, 2017

@LordGuilly Can you please sign https://developer.mbed.org/contributor_agreement/ ? And share your nick on mbed?

@0xc0170 0xc0170 added the needs: CLA label Jul 14, 2017

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 14, 2017

my mbed user is 'guilly' not sure if it can be linked to the github profile?

@0xc0170 0xc0170 removed the needs: CLA label Jul 14, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 14, 2017

/morph test

1 similar comment
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 17, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 817

All builds and test passed!

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 17, 2017

Hi
Note that ST driver team doesn't approve this update...

I suggest to remove the commit in stm32f1xx_hal_rcc.h file from this pul request, and to keep only the patch in targets/TARGET_STM/TARGET_STM32F1/TARGET_BLUEPILL_F103C8/device/system_stm32f1xx.c

This will make Bluepill target works anyway.

In //, I am going to work on a patch which will satisfy everyone and every STM32 family (not only F1)

Thx

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 17, 2017

I suggest to remove the commit in stm32f1xx_hal_rcc.h file from this pul request, and to keep only the patch in targets/TARGET_STM/TARGET_STM32F1/TARGET_BLUEPILL_F103C8/device/system_stm32f1xx.c

This will make Bluepill target works anyway.

@LordGuilly What do you think, can you do the update?

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Jul 17, 2017

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 17, 2017

what's the problem, side effects? Or is it also present in other platforms, and want to solve it for all at the same time?
Do you need me to remove the commit, or can be done automatically?

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 17, 2017

what's the problem, side effects?

They mention 2 points:

  • in some cases, when user call twice HAL_RCC_OscConfig() with HSE On, in each call the HSE will be disabled then enabled

  • after clearing HSEON and HSEBYP, and before setting HSEON, it seems that we have to wait on HSERDY= 0 to guarantee the sequence

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 17, 2017

well, make sense, I guess the best approach would be to replace the macro with an inline call, and add all the extra logic. Doing it with a macro will not be very clear.
Disabling the HSE is needed only if HSEBYP is enabled, because the bit cannot be changed with the HSE on.
I will update the PR to have only the BLUEPILL specific commit, likely will test it tonight at home.

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 17, 2017

reverted the commit, tested with my board, and definitely works, but only for this platform at the moment.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 17, 2017

@LordGuilly I think the history would be cleaner if you just rebased/squashed and removed the commit that you reverted and the revert.

@LordGuilly LordGuilly force-pushed the LordGuilly:bluepill_hse_config branch Jul 17, 2017

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 17, 2017

I think I did it right this time ;-)

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 17, 2017

Looks great!

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Jul 18, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 18, 2017

/morph test

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jul 18, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 19, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 845

Test failed!

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Jul 20, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 20, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 854

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jul 24, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 24, 2017

Sorry @LordGuilly It looks like the pile of merges that got in before this one caused a merge conflict. Could you rebase to resolve this conflict?

Change HSE for blupill,can only be XTAL
    The BLUEPILL board does have XTAL soldered, cannot be used with an external oscillator

@LordGuilly LordGuilly force-pushed the LordGuilly:bluepill_hse_config branch to 909a834 Jul 24, 2017

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 24, 2017

done it, the file tree was restructured, and the fix had to be done in another file

@adustm

This comment has been minimized.

Member

adustm commented Jul 25, 2017

Hello @LordGuilly
Thank you for the fix.
I only have a comment on your PR's details:

it may impact other F1xx branches,

As you modify only TARGET_STM32F1/TARGET_BLUEPILL_F103C8/device/system_clock.c, it means that only BLUEPILL_F103C8 device will be affected by your modification.
Kind regards
Armelle

@LordGuilly

This comment has been minimized.

Contributor

LordGuilly commented Jul 25, 2017

Yes, the comment is a leftover from the original PR, which also included changes in a generic f1xx header. Those changes were removed due to ST driver team objections

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Jul 25, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 25, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jul 26, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 887

All builds and test passed!

@0xc0170 0xc0170 changed the title from Bluepill hse config to Bluepill: hse config fix Jul 26, 2017

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jul 26, 2017

@theotherjimmy theotherjimmy merged commit 66805f5 into ARMmbed:master Jul 27, 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
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jul 27, 2017

Thanks for the fix @LordGuilly!

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