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

Stm32 pwm complementary channel #1018

Merged
merged 7 commits into from
May 18, 2023

Conversation

ser-plu
Copy link

@ser-plu ser-plu commented May 9, 2023

#1011
I implemented the fixes only for the G4 family.

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

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

Thanks!

src/modm/platform/timer/stm32/general_purpose.cpp.in Outdated Show resolved Hide resolved
src/modm/platform/timer/stm32/general_purpose.hpp.in Outdated Show resolved Hide resolved
src/modm/platform/timer/stm32/general_purpose_base.hpp.in Outdated Show resolved Hide resolved
@ser-plu ser-plu force-pushed the stm32-pwm-complementary-channel branch 3 times, most recently from fa9b4ce to a149573 Compare May 10, 2023 08:48
@chris-durand chris-durand force-pushed the stm32-pwm-complementary-channel branch from 885e24d to 121f4ad Compare May 10, 2023 19:28
@chris-durand
Copy link
Member

Github is having some outage right now. CI jobs randomly fail.

@chris-durand
Copy link
Member

@ser-plu Could you add yourself to the copyright headers in general_purpose_base.hpp.in / general_purpose_base.cpp.in? Then the changes look good to be merged from my side.

@chris-durand chris-durand added this to the 2023q2 milestone May 10, 2023
@chris-durand chris-durand added the ci:hal Triggers the exhaustive HAL compile CI jobs label May 10, 2023
@ser-plu
Copy link
Author

ser-plu commented May 10, 2023

Will do. I would also like to add a function isComplementarySignal to the interface. And maybe a couple of functions with Timer properties, like isAdvancedTimer etc. Then I can implement a very generic abstractions, that could use any timer available

@ser-plu ser-plu force-pushed the stm32-pwm-complementary-channel branch 2 times, most recently from 13430c8 to 8fe6fcf Compare May 11, 2023 13:22
@ser-plu
Copy link
Author

ser-plu commented May 11, 2023

Guys, could you check the CI here as well? It failed in different places, probably due to Github outages

@rleh
Copy link
Member

rleh commented May 11, 2023

I restarted the failed jobs :)

The docs-job fails for the same reason as #1017, lets fix the issue over there and rebase this branch later.

@salkinium
Copy link
Member

CI is fixed now.

@ser-plu ser-plu force-pushed the stm32-pwm-complementary-channel branch from 8fe6fcf to 8053eda Compare May 15, 2023 09:02
@ser-plu
Copy link
Author

ser-plu commented May 15, 2023

Seems to be ready to be merged

@chris-durand chris-durand force-pushed the stm32-pwm-complementary-channel branch from 8053eda to a3d21e3 Compare May 16, 2023 23:26
@chris-durand chris-durand added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels May 16, 2023
@ser-plu
Copy link
Author

ser-plu commented May 17, 2023

I have seen, that @chris-durand removed a couple of functions, that I had added before (isComplementaryChannel, Timer::hasAdvancedControl). I wanted to use them in a generic PWM channel like here:
develop...ser-plu:modm:generic-pwm-channel
(the code is does not correspond to your codestyle, I wanted to adapt it after we finish with this PR)

With this code one could use PWM output like this:

using Led1Pwm = PwmChannel<Timer16, GpioOutputB6::Ch1n, SystemClock>;
using MotorControlPwm = PwmChannel<Timer3, GpioOutputA6::Ch1, SystemClock>;
using Dac1OutputPwm = PwmChannel<Timer8, GpioOutputB1::Ch3n, SystemClock>;
using Led2Pwm = PwmChannel<Timer17, GpioOutputA7::Ch1, SystemClock>;

Led1Pwm led1;
MotorControlPwm motor;
Dac1OutputPwm dac1;
Led2Pwm led2;

auto pwm_channels = std::to_array<IPwmChannel* const>({&led1, &motor, &dac1, &led2});

for (auto* chan : pwm_channels) {
  chan->Init();
}

while (1) {
  for (auto* chan : pwm_channels) {
    chan->SetDutyCycle(value);
  }
}

I find it more convenient, than writing the same lines for every PWM I need, also thinking every time, whether the Timer has MOE bit or not and remembering about the complementeary channel pins.
Also having the Interface as a base class helps in Unit tests of the code, that uses the PWM.

So I suggest to bring my functions back into PR :)

@chris-durand
Copy link
Member

chris-durand commented May 17, 2023

Oh, I must have messed something up by accident. I only wanted to rebase, sorry. Could you push your changes again?

EDIT: I found the terminal from yesterday. I did a git fetch but it failed with a network error and I didn't notice 🙈.

@ser-plu
Copy link
Author

ser-plu commented May 17, 2023

returned the changes

@chris-durand chris-durand added ci:hal Triggers the exhaustive HAL compile CI jobs and removed ci:hal Triggers the exhaustive HAL compile CI jobs labels May 17, 2023
@salkinium salkinium merged commit 9dc4f19 into modm-io:develop May 18, 2023
@salkinium salkinium linked an issue May 18, 2023 that may be closed by this pull request
@ser-plu ser-plu deleted the stm32-pwm-complementary-channel branch May 19, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:hal Triggers the exhaustive HAL compile CI jobs fix 💎
Development

Successfully merging this pull request may close these issues.

STM32G4: modm interface hides some of Timer features
4 participants