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

Scale fan setting to match PWM_F #20

Merged
merged 4 commits into from
Jan 10, 2022
Merged

Conversation

stoklund
Copy link
Contributor

When converting a fan speed percentage to an LSB byte written to the EMC2101 controller, the conversion should translate 100% to 2*PWM_F because the chip determines the PWM duty cycle that way.

The changes in this PR calculate and cache the byte value to use for 100% fan speed in the function _calculate_full_speed(). This cached full speed value is recomputed whenever emc.pwm_frequency or emc.dac_output_enabled are changed — these are the two registers affecting the value.

When setting emc.manual_fan_speed or one of the LUT values, the cached full speed value is used to compute the byte to be programmed into the chip. This means that emc.pwm_frequency should always be set before programming the LUT or manual speed setting.

My new oscilloscope arrived today (yay!), so I was able to test this code by measuring the duty cycle on the FAN pin. I tested 10% fan speed increments both with the default PWM frequency, and with PWM_F=5 which produces a 35 kHz PWM signal.

pwm_f5.pdf

Both PWM frequencies plot as a straight line. At 35 kHz, I measured a duty cycle about 0.75% less that the programmed value. I think this is caused by the 1 µs rise time on the open drain FAN pin.

This was previously fixed in adafruit#4, but it looks like the fix got clobbered when the LUT code was split out to a separate file.
This conversion is going to depend on the PWM_F and DAC register values, so it needs
to be a method. Use this method in `manual_fan_speed()` as well as the LUT subclass.

Move the `_pwm_freq` register definition into the base class too. It will be
needed for the fixed fan speed conversions.

Also import the `MAX_LUT_*` constants from the base module rather than redefining them.
- Add a `_full_speed_lsb` member variable which represents the (smallest) LSB
  value corresponding to a 100% fan setting.
- Add a `_calculate_full_speed()` method that computes it.
- Use `_full_speed_lsb` when converting percentages to fan speed settings and back.

Fixes adafruit#19.
Recalculate the full speed LSB setting when this property is changed too.
@ladyada ladyada requested a review from a team November 1, 2021 19:13
@ladyada
Copy link
Member

ladyada commented Nov 1, 2021

thanks! hopefully someone from the circuitpy community could test this PR and then we will merge

@askpatrickw
Copy link

@stoklund is this a fix for #19 ?

@stoklund
Copy link
Contributor Author

stoklund commented Nov 6, 2021

@stoklund is this a fix for #19 ?

Yes

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I tested this branch successfully with a Feather RP2040 + EMC2101 + a 5v Raspberry Pi cooling fan. All examples in the repo seem to work as expected.

@FoamyGuy FoamyGuy merged commit 43cc811 into adafruit:main Jan 10, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 10, 2022
Updating https://github.com/adafruit/Adafruit_CircuitPython_DPS310 to 2.1.0 from 2.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_DPS310#20 from FoamyGuy/default_import

Updating https://github.com/adafruit/Adafruit_CircuitPython_EMC2101 to 1.1.10 from 1.1.9:
  > Merge pull request adafruit/Adafruit_CircuitPython_EMC2101#20 from stoklund/full_speed
  > update rtd py version

Updating https://github.com/adafruit/Adafruit_CircuitPython_HT16K33 to 4.1.9 from 4.1.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_HT16K33#95 from tekktrik/doc/update-documentation
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.

4 participants