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 pwm period calc #3795

Merged
merged 6 commits into from Mar 9, 2017

Conversation

Projects
None yet
4 participants
@LMESTM
Contributor

LMESTM commented Feb 17, 2017

Description

This pull request solves an issue that was encountered in specific cases where pwm HW frequency is high and the pwm period was about 100ms (encountered on F746). This was a limitation due to hard coded pre-scaler values of 1 or 500 only. The solution is to have more granularity.

In order to apply the fix to all families and not only F7, the code is also made common between all families which makes maintenance easier.

Status

READY

TESTS RESULTS

pwm_test_results.txt

LMESTM added some commits Feb 16, 2017

STM32 L1: Define PWM Channels in PeripheralsPins.c
As done for other families, let's define the PWM channel in the PWM
pins table definition rather than driver.
STM32: make PWM driver into a common file
The pwmout driver is very similar for each STM32 family.

The only family specific part is defined in pwmout_device.h file.
It mainly contains few specific information:
- The mapping of PWM/TIMERS to APB1 or APB2 so that we can get the clock
- The clock calculation uses the right APB clock, which was sometimes
not the case before and could have lead to errors in case dividers were
enabled on APB clock settings. This case is now covered.
- Inactivation of inverted support on feaw families
TimHandle.Init.Period = (us - 1);
/* In case period or pre-scalers are out of range, loop-in to get valid values */
while ((TimHandle.Init.Period > 0xFFFF) || (TimHandle.Init.Period > 0xFFFF)) {

This comment has been minimized.

@bcostm

bcostm Feb 17, 2017

Contributor

same test

This comment has been minimized.

@LMESTM

LMESTM Feb 17, 2017

Contributor

oops - needs to be corrected
will do when I'm back in office

This comment has been minimized.

@0xc0170

0xc0170 Feb 20, 2017

Member

Any update for this?

This comment has been minimized.

@bcostm

bcostm Feb 21, 2017

Contributor

@LMESTM is off this week.

/* In case period or pre-scalers are out of range, loop-in to get valid values */
while ((TimHandle.Init.Period > 0xFFFF) || (TimHandle.Init.Period > 0xFFFF)) {
obj->prescaler = obj->prescaler * 2;
if (APBxCLKDivider == RCC_HCLK_DIV1)

This comment has been minimized.

@0xc0170

0xc0170 Feb 20, 2017

Member

if you are touching this file, please fix formatting if possible

This comment has been minimized.

@LMESTM

LMESTM Feb 27, 2017

Contributor

@0xc0170 I had a quick look but probably missed the point - what's wrong with current formatting ?

This comment has been minimized.

@0xc0170

0xc0170 Feb 27, 2017

Member

should be

if (condition) {
  do();
} else {
  do_else();
}

{ and } are missing there

PwmoutApb pwmoutApb;
} pwm_apb_map_t;
static const pwm_apb_map_t pwm_apb_map_table[] =

This comment has been minimized.

@0xc0170

0xc0170 Feb 20, 2017

Member

I believe there was earlier PR about 2 weeks ago that was defining some variables in the header file. I raised an issue there that this would be much better if it's done in a code file.
It works now as it is included in just one code file. however, this might become a silent bomb later if this HAL will include a new code file that would require some declarations from this file (it's possible and might be overlooked that this actually defines this map of pwm).

This comment has been minimized.

@LMESTM

LMESTM Feb 27, 2017

Contributor

@0xc0170 yes you're right ! I will move the definition to a C file

@0xc0170 0xc0170 added the needs: work label Feb 20, 2017

LMESTM added some commits Feb 27, 2017

STM32: pwm period and prescaler calculation
Correct the while loop limit and add a safe guard to avoid infinite loop.
STM32: move pwmout device tables to C file
In order to avoid possible multiple definitions errors, move the table
initialization to the C file instead of header file
STM32: fix formatting
Use the recommended style
if (condition) {
  do();
} else {
  do_else();
}
@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Feb 27, 2017

@0xc0170 @bcostm branch updated as per your comments

@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Mar 6, 2017

@0xc0170 hey - I think the PR now takes into account your comments - let me know if anything missing

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 6, 2017

@0xc0170

0xc0170 approved these changes Mar 6, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 6, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 6, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1641

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 7, 2017

@LMESTM Please look at the failures

Here's the output (XDOT platform fails for all 3 toolchains)

13:29:01         [Error] PeripheralPins.c@88,0:  #54-D: too few arguments in invocation of macro "STM_PIN_DATA_EXT"
13:29:01         [Error] PeripheralPins.c@88,0:  #158: expression must be an lvalue or a function designator
13:29:01         [Error] PeripheralPins.c@88,0:  #158: expression must be an lvalue or a function designator
13:29:01         [Warning] PeripheralPins.c@88,0:  #1278-D: excess initializers are ignored

The link is above in the failure message to get this log

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Mar 7, 2017

Fix XDOT compilation error
Typo with misplaced closing parenthesis leads to compilation error,
which is fixed with this patch
@LMESTM

This comment has been minimized.

Contributor

LMESTM commented Mar 7, 2017

@0xc0170 - thanks - there was a typo in this XDOT file. Fixed now.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Mar 8, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 8, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1654

All builds and test passed!

@0xc0170 0xc0170 merged commit fd6fdd5 into ARMmbed:master Mar 9, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has started
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