Skip to content

Add I2C bitbanging driver capable of retry upon NACK#157

Closed
HarryMakes wants to merge 5 commits intodevelopfrom
feature/i2c-bitbang
Closed

Add I2C bitbanging driver capable of retry upon NACK#157
HarryMakes wants to merge 5 commits intodevelopfrom
feature/i2c-bitbang

Conversation

@HarryMakes
Copy link
Copy Markdown
Contributor

@HarryMakes HarryMakes commented Sep 27, 2021

Summary

This PR adds an I2C master driver implemented with bitbanging and retry mechanism, which can be enabled on all I2C devices via a new crate feature named i2c_bitbang.

Motivation

#140 describes an issue on I2C writes operated on the chassis fan controllers, and it has been observed on multiple Booster units at various timestamps since boot-up. Based on the discussion and my own experiment, it does not seem to arise from the default I2C controller on the STM32 MCU, as a custom-made bitbanging driver does not suppress the issue. Rather, the fan controller I2C slave seems to occasionally emit NACK upon I2C writes. Right now, the Rust HAL simply returns the NACK error, and the NGFW panics.

As a workaround, on top of bitbanging, this PR introduces a retry mechanism upon such NACK. With my own reading of related specifications documents, the I2C master is expected to perform a repeated START after NACK, so it is a reasonable addition. Please kindly follow my comment (link) for further explanation.

Testing

This PR was first tested by isolating all other scheduled tasks (e.g. MQTT telemetry, channel monitoring, USB terminal), using a debugger. Test runs showed no panics or watchdog triggers.

The latest test run used a modified version of 9602b9c that incorporates ENC424J600 (#156). This was performed as the temperature reading of each channel was logged from the MQTT telemetry, and lasted for more than a week without any panics or watchdog triggers. The plot of temperature readings show that the fan levels were properly adjusted and stayed below the default threshold of 30°C + 3°C of error (according to chassis_fans.rs:88).

Related Issues

This closes #140 when the I2C bitbang driver is used.

@HarryMakes
Copy link
Copy Markdown
Contributor Author

16f3de2 was pushed to edit the GitHub workflows matrix, in a fashion similar to 8055090.

@HarryMakes
Copy link
Copy Markdown
Contributor Author

2bc0acf simplifies the workflow in a way similar to 38fa6f4.

@jordens
Copy link
Copy Markdown
Member

jordens commented Sep 30, 2021

I don't understand why this is the right way to go.

Why do we need a I2C bitbanging driver? Can't we retry the transaction with the stm32 peripheral in the same way? If that's possible, that should be the way to go.
Also if you want to write a i2c bitbanging driver, that is all good and useful. But it should likely go into its own crate and then can be used here. Reviewing and maintaining it here sounds like an unnecessary burden.
This is all before reviewing the correctness of the I2C implementation.

@HarryMakes
Copy link
Copy Markdown
Contributor Author

@jordens Thank you for the alarm. I have created a new PR just for the retry mechanism and for setting the chassis fan speed.

I'll close this PR. I can also remove my feature/i2c-bitbang branch here for now, unless it becomes worth investigating. The motivation of bitbanging was to check for possible bugs in the STM32 I2C controller in our Booster application, but it's apparent that the chassis fan NACK issue does not originate there.

@HarryMakes HarryMakes closed this Sep 30, 2021
@HarryMakes HarryMakes deleted the feature/i2c-bitbang branch September 30, 2021 09:10
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.

NACK encountered while communicating with fan controllers

2 participants