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

Bang & Olufsen support plus higher precision and frequency PWM generation #2074

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

danielwallner
Copy link

Bang & Olufsen 455 kHz protocol support with additional improvements to the PWM generation.
Uses fixed point pulse widths for higher precision and polling of micros instead of delay to be able to reach the frequencies required.
On ESP8266 lower than 160 MHz a free running PWM mode was also added to get as close to 455 kHz as possible.
A bonus is that the lower frequencies now are accurate to within a few hertz.
Some additional commits were necessary to be able to run the tests with -Wall -Wextra -Werror on macOS.
The commit that comments unused constants may need a bit of cleanup.
Note that the current test isn't compatible with micros polling and the old code is used for tests.

@NiKiZe
Copy link
Collaborator

NiKiZe commented Feb 25, 2024

Might it be worth doing separate PRs for the commits to see that each step is OK?
Especially the sending change and the added protocol.

First 2 commits looks good

The Cast looks like s bug and should be investigated some more.

As you say the consts needs cleanup when using /* */ use separate lines to not modify the existing ones, but if truly unused we should consider removing them if they do not provide any value in some other way.

Having different variants between ESP and tests does reduce the reason to have tests? Should/could we consider some other way of dealing with that?

@crankyoldgit crankyoldgit self-requested a review February 25, 2024 21:56
@danielwallner
Copy link
Author

Ok, I'll try to separate this into more than one PR.

Yes, the cast definitely looks like a bug.

What should I do with the unused constants?
Some of them are explicitly commented as being there for documentation.
Does that apply to all of them or should they just be removed if unused?

I can't think of a way to test the PWM that doesn't include ifdefs with delays inside the PWM loop.
That's probably better than essentially disabling the test as it now though.

Also, I forgot to mention that the calibration no longer does anything but was kept to not break things.

There's also an additional test failure on macOS in IRac_test that I didn't look into.

@danielwallner
Copy link
Author

I now what the IRac_test failure is now.
_swingh_prev is not reset in IRLgAc::stateReset.
But I also noticed that ir_Delonghi_test fails about once every five runs too.
The message in TestDecodeDelonghiAc is decoded as unknown when it fails.
I have not been able to find the cause for this.
I also haven't checked if this is a problem with clang everywhere or just on macOS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants