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

Reimplement simple pwm #272

Merged
merged 10 commits into from
Jul 4, 2022
Merged

Reimplement simple pwm #272

merged 10 commits into from
Jul 4, 2022

Conversation

dalpil
Copy link
Contributor

@dalpil dalpil commented May 15, 2022

This is a reimplementation of the "simple" fastpwm functionality from the old-branch.

I've only been able to test this on a Arduino Uno, a Leonardo and a Trinket, but I've tried to add implementations for some of the other MCUs (atmega48p, atmega168, atmega328pb, atmega2650) as well, based on the code in the old-branch.

I also added implementations for atmega32u8, attiny85, attiny88 and atmega1280 as well, which wasn't available in the old branch IIRC.

Any feedback would be greatly appreciated.

Closes #251.

@dalpil
Copy link
Contributor Author

dalpil commented May 15, 2022

Fixed my typo which tried to use simple_pwm on all MCU types instead of just the atmega ones.

@Rahix Rahix added the hal-api API design for the different components of avr-hal label May 16, 2022
@dalpil dalpil marked this pull request as draft May 31, 2022 11:49
@dalpil
Copy link
Contributor Author

dalpil commented May 31, 2022

This PR should probably have been a Draft from the beginning, sorry about the noise..

I'll add simple_pwm implementations for the remaining MCU's and fix the documentation errors that I've made this coming weekend.

@dalpil dalpil marked this pull request as ready for review June 4, 2022 11:25
@dalpil
Copy link
Contributor Author

dalpil commented Jun 4, 2022

I didn't set out to implement this for all the (currently) supported MCUs from the beginning, but it was rather fun, so I just kept going.

This should now be fairly "complete" as far as the current MCU support goes.

The tests break because they depend on a new release of avr-device (due to the missing timer-patches for atmega1280), so I guess the "waiting-for-avr-device" label would be appropriate here.

Pros

  • I can now fade my LEDs.
  • The end-user API stays the same as on the old-branch, which makes it easier to port my projects.
  • I learned a ton about the different timers on AVR-chips just by skimming the datasheets.

Cons

  • I'm not sure if this thin abstraction will fit the 4809. Those timers are configured in a totally different way, and since I lack the necessary hardware, I haven't been able to take a stab at it at all.
  • This is just plain old FastPWM, which now feels a bit "meh", considering how much cool stuff you can do with the rest of the timers and/or modes on AVRs. So I guess this is more of a stopgap between no PWM API at all, and a fully fleshed out PWM API with support for phase-correct wave generation modes, frequency control, PLLs, dead-time configuration and what have you. This is being discussed in other issues; Accurate PWM #44 for example.
  • Adding this as a "weak" API right now might cause headaches/churn further down the road, if a more powerful API come along to replace it. Especially since I'm not sure that it'll fit the newer MCUs (4809 et.al).

As said; any feedback is greatly appreciated.

@Rahix Rahix added the waiting-for-avr-device Waiting for a new release of `avr-device` label Jun 5, 2022
@rogday
Copy link

rogday commented Jun 18, 2022

Thank you SO MUCH for this PR! I know nothing about microcontrollers and that means I'm hesitant to use workaround from other issues that uses unfamiliar for me internal names. Using this branch I was able to play with my LED too :D

@Rahix
Copy link
Owner

Rahix commented Jun 26, 2022

Hey @dalpil, with #283 the new version of avr-device is now used. Please rebase your PR ontop of the latest main to pull in this change and hopefully get CI to pass this way.

Sorry for the slow progress from my side here...

@Rahix Rahix removed the waiting-for-avr-device Waiting for a new release of `avr-device` label Jun 26, 2022
@dalpil dalpil force-pushed the reimplement-simple-pwm branch from 69c9d9b to e615e7e Compare June 26, 2022 11:02
@dalpil
Copy link
Contributor Author

dalpil commented Jun 26, 2022

No worries, I'm guessing that you maintain these libraries as a hobby, and it wasn't my intention to cause any stress by opening a fairly big unsolicited pull request. I'm just happy to tinker with this. :)

@dalpil dalpil force-pushed the reimplement-simple-pwm branch from e615e7e to d89427a Compare June 28, 2022 11:57
@dalpil
Copy link
Contributor Author

dalpil commented Jun 28, 2022

Rebased once more to add simple_pwm for the attiny84.

Copy link
Owner

@Rahix Rahix left a comment

Choose a reason for hiding this comment

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

Hey, thank you very much again for this changeset! Sorry for all the delays from my side...

I took the time to review it now, everything looks good enough that I will merge it as is. I think it is better to get this into the project ASAP and iterate on anything else in the future.

Thanks btw for the cleanly rebased commit series. Not many people take the time to do this so I appreciate it a lot.

I tested the Uno example. It works fine but for some reason the built-in LED on pin 13 seems to behave weirdly while it runs... Configuring the pin as output to drive it explicitly fixes this. But I do not think we should worry about it here.

Finally one minor nit for the future: You should configure your editor to always save files with a trailing newline. In some files here, it is missing, but I will fix this for you after merging.

@Rahix Rahix merged commit 754619f into Rahix:main Jul 4, 2022
@jeroenvlek
Copy link
Contributor

Thank you very much for picking this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hal-api API design for the different components of avr-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Analog output for Arduino Uno
4 participants