Skip to content

[RUMBA32] Minor Improvements and Fixes #1092

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

Merged
merged 7 commits into from
Jun 12, 2020

Conversation

chrissbarr
Copy link
Contributor

@chrissbarr chrissbarr commented Jun 8, 2020

[RUMBA32] Minor Improvements and Fixes

Summary

The fixes relating to timer conflicts are raised as a result of the investigation conducted here: Aus3D/RUMBA32#26

This PR fixes/implements the following changes:

  • Update PWM pin mapping
  • Set TIMER_SERIAL to TIM9 to remove conflict with TIMER_SERVO (refer to Variant.h for RUMBA32 #731 and Fan PWM Troubleshooting / Timer Investigation Aus3D/RUMBA32#26)
    • Check if TIMER pin mapping can be overridden by user
  • Enable ADC mapping on all pins which map to ADC1 (original A0 - A6 pin mapping is preserved and there should be no impact to existing software that uses those pin names)
  • Enable UART5 on EXP1 header
  • Tidy variant files and add peripheral options from current genpinmap file (a few additional UART, SPI etc. options are in the current file, added)

I am curious, does anyone know if there's a way to define the timers in the variant.h file such that they can be overridden by the user's code? For instance, I tried setting up the variant.h file like:

#ifndef TIMER_SERIAL
  #define TIMER_SERIAL           TIM9
#endif

But it seems that the variant.h file gets compiled before anything I do in the Arduino sketch / header, and I see a warning that TIMER_SERIAL is redefined. Is there a way to make this work? Would be quite useful if the TONE, SERVO and SERIAL timers could all be redefined by the user, while still allowing for a variant default.

Validation

  • Ensure CI build is passed.
  • Demonstrate the code is solid. [e.g. Provide a sketch]

Code formatting

  • Ensure AStyle check is passed thanks CI

Closing issues

- Fix timer conflicts in PWM pin mapping
- Enable PWM on expansion pins
- PWM removed from some pins that don't require it
@fpistm
Copy link
Member

fpistm commented Jun 8, 2020

I am curious, does anyone know if there's a way to define the timers in the variant.h file such that they can be overridden by the user's code? For instance, I tried setting up the variant.h file like:

#ifndef TIMER_SERIAL
  #define TIMER_SERIAL           TIM9
#endif

But it seems that the variant.h file gets compiled before anything I do in the Arduino sketch / header, and I see a warning that TIMER_SERIAL is redefined. Is there a way to make this work? Would be quite useful if the TONE, SERVO and SERIAL timers could all be redefined by the user, while still allowing for a variant default.

Well, in this case you could redefine it at the sketch level, using the build_opt.h with:
-DTIMER_SERIAL=TIM9

https://github.com/stm32duino/wiki/wiki/Customize-build-options-using-build_opt.h

Make format consistent with that recommended in board_template when not using NUM_ANALOG_FIRST.

Groundwork for enabling further ADC pins in future commit.
Enable analog input on all pins which map to ADC1.
Maintain consistency with previous pin mapping of A0 - A6, so should be no impact to existing code that uses this mapping.
Add extra possible configurations from current STM32F446V(C-E)Tx genpinmap PeripheralPins.c file.
User-accessible via EXP1 header.
Default TIMER_SERIAL assignment is to TIM7, which conflicts with TIMER_SERVO.
@chrissbarr chrissbarr force-pushed the RUMBA32-improvements branch from c84348c to 28dadd2 Compare June 9, 2020 10:49
@chrissbarr
Copy link
Contributor Author

Well, in this case you could redefine it at the sketch level, using the build_opt.h with:
-DTIMER_SERIAL=TIM9

Hmm, it might be worth surrounding each of the timers defined in variant.h with an #ifndef check then, so that they can be overridden in this way if desired. Are you aware of any disadvantages to doing this? If not, I think I'll add a commit doing this for serial/servo/tone timers.

@fpistm
Copy link
Member

fpistm commented Jun 9, 2020

Hmm, it might be worth surrounding each of the timers defined in variant.h with an #ifndef check then, so that they can be overridden in this way if desired. Are you aware of any disadvantages to doing this? If not, I think I'll add a commit doing this for serial/servo/tone timers.

From my point of view there is no issue to add #ifndef.

@fpistm fpistm added the enhancement New feature or request label Jun 9, 2020
@fpistm fpistm added this to the 2.0.0 milestone Jun 9, 2020
@fpistm fpistm self-requested a review June 9, 2020 15:34
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

LGTM.

@chrissbarr
Copy link
Contributor Author

Thanks @fpistm. I've noticed a bit of weirdness when using SoftwareSerial with certain pins, and have opened an issue about this (#1093). Hope you can take a look when you get the opportunity.

I might hold off on requesting a merge on this until I understand what's happening there, in case it's something I've introduced or can fix in the pin definitions.

Allow TIMER_TONE, TIMER_SERVO and TIMER_SERIAL to be overridden using built_opt.h or similar.
@chrissbarr chrissbarr force-pushed the RUMBA32-improvements branch from 1dfd0b9 to f13ec1e Compare June 10, 2020 06:06
@chrissbarr
Copy link
Contributor Author

All right, I'm satisfied with these changes and am happy for the PR to be merged. I'll move it from draft now and it is yours to review @fpistm!

@chrissbarr chrissbarr marked this pull request as ready for review June 11, 2020 11:50
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

LGTM

@fpistm fpistm merged commit 443ede7 into stm32duino:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants