chassis_fans: Retry set_duty_cycles() upon NACK#158
Merged
ryan-summers merged 29 commits intodevelopfrom Oct 6, 2021
Merged
Conversation
(afdab5f) cargo: include enc424j600 dependencies (39e0b55) main: feature gate different eth device (e845f56) nal: implement enc424j600 variant (befe70a) nal: fix unnecessary w5500 dependencies (20efdcc) settings: override eeprom MAC (0d90470) use cargo fmt (6e13f4f) cargo: add w5500 as default (83380a9) cargo: simplify smoltcp dependency (83109c3) settings/mac: remove redundant code (a3db62f) mac: use smoltcp (15f0964) eth_cfg: split off from main (4ae6e82) nal: migrated to enc424j600 (e66d916) phy_config: minor fix (3869217) clean up
* This also tells the ENC424J600 driver to use Cortex-M instructions to perform SPI CS_n delays at the current firmware CPU clock frequency.
* mqtt_control: Also fix missing feature gate for W5500
ryan-summers
requested changes
Sep 30, 2021
Add support for ENC424J600 with bumped minimq
Updating changelog for PHY addition
jordens
reviewed
Oct 2, 2021
jordens
reviewed
Oct 2, 2021
ryan-summers
reviewed
Oct 4, 2021
ryan-summers
requested changes
Oct 6, 2021
Member
ryan-summers
left a comment
ryan-summers
left a comment
There was a problem hiding this comment.
One last little nitpick, but otherwise this PR looks good-to-go for me.
Member
|
@HarryMakes Oh, and please add a |
Contributor
Author
|
@ryan-summers To change |
Member
|
I prefer merging into |
Contributor
Author
|
Added line to the Changelog with caea06f. Thanks! |
ryan-summers
approved these changes
Oct 6, 2021
Member
ryan-summers
left a comment
ryan-summers
left a comment
There was a problem hiding this comment.
Don't know why Github is showing a lot of changed files... Manual compare shows a much more reasonable changeset. Everything looks good to me though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a retry mechanism for NACK encountered while setting the MAX6639 fan speed.
Motivation
This implements the same retry mechanism as #157, except that this PR sticks with the STM32 HAL I2C driver and only considers
Max6639::set_duty_cycle().Just like #157, this aims to tackle #140 regarding random occurrence of NACK upon I2C writes to the chassis fan controllers. My original findings (see comment) supports that this retry mechanism is a good but dirty solution.
Besides, there have not been reports of similar occurrence of NACK for any other operations on MAX6639 or any other I2C devices. Thus, it should be reasonable to only implement retries for just
ChassisFans::set_duty_cycles().Testing
At the time of opening this new PR, there has not been prolonged test runs for this retry mechanism with the STM32 I2C controller. However, my bitbanging driver has empirically shown that the retry mechanism is reliable, e.g. does not trigger the watchdog. Further testing is welcome and desirable.
Related Issues
This closes #140.