76 lines
2.8 KiB
Markdown
76 lines
2.8 KiB
Markdown
|
# Phosphor-hwmon refactoring
|
||
|
|
|
||
|
|
Author: Kun Yi <kunyi!>
|
||
|
|
|
||
|
|
Created: 2019/09/05
|
||
|
|
|
||
|
|
## Problem Description
|
||
|
|
|
||
|
|
Convoluted code in phosphor-hwmon is imparing our ability to add features and
|
||
|
|
identify/fix bugs.
|
||
|
|
|
||
|
|
## Background and References
|
||
|
|
|
||
|
|
A few cases of smelly code or bad design decisions throughout the code:
|
||
|
|
|
||
|
|
- Limited unit testing coverage. Currently the unit tests in the repo [1] mostly
|
||
|
|
test that the sensor can be constructed correctly, but there is no testing of
|
||
|
|
correct behavior on various sensor reading or failure conditions.
|
||
|
|
- Bloated mainloop. mainloop::read() sits at 146 lines [2] with complicated
|
||
|
|
conditions, macros, and try/catch blocks.
|
||
|
|
- Lack of abstraction and dependency injection. This makes the code hard to test
|
||
|
|
and hard to read. For example, the conditional at [3] can be replaced by
|
||
|
|
implementing different sensor::readValue() method for different sensor types.
|
||
|
|
|
||
|
|
[1](https://github.com/openbmc/phosphor-hwmon/tree/master/test)
|
||
|
|
[2](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L390)
|
||
|
|
[3](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L408)
|
||
|
|
|
||
|
|
## Goals
|
||
|
|
|
||
|
|
1. Improve unit test coverage
|
||
|
|
2. Improve overall design using OOD principles
|
||
|
|
3. Improve error handling and resiliency
|
||
|
|
4. Improve configurability
|
||
|
|
- By providing a common configuration class, it will be feasible to add
|
||
|
|
alternative configuration methods, e.g. JSON, while maintaining backward
|
||
|
|
compatibility with existing env files approach.
|
||
|
|
|
||
|
|
## Proposed Design
|
||
|
|
|
||
|
|
Rough breakdown of refactoring steps:
|
||
|
|
|
||
|
|
1. Sensor configuration
|
||
|
|
- Add a SensorConfig struct that does all std::getenv() calls in one place
|
||
|
|
- DI: make the sensor struct take SensorConfig as dependency
|
||
|
|
- Unit tests for SensorConfig
|
||
|
|
- Add unit tests for sensor creation with various SensorConfigs
|
||
|
|
2. Abstract sensors
|
||
|
|
- Refine the sensor object interface and make it abstract
|
||
|
|
- Define sensor types that inherit from the common interface
|
||
|
|
- Add unit tests for sensor interface
|
||
|
|
- DI: make the sensor map take sensor interface as dependency
|
||
|
|
- Add a fake sensor that can exhibit controllable behavior
|
||
|
|
3. Refactor sensor management logic
|
||
|
|
- Refactor sensor map to allow easier lookup/insertion
|
||
|
|
- Add unit tests for sensor map
|
||
|
|
- DI: make all other functions take sensor map as dependency
|
||
|
|
4. Abstract error handling
|
||
|
|
- Add overridable error handler to sensor interface
|
||
|
|
- Define different error handlers
|
||
|
|
- Add unit tests for error handlers using the fake sensor
|
||
|
|
|
||
|
|
## Alternatives Considered
|
||
|
|
|
||
|
|
N/A.
|
||
|
|
|
||
|
|
## Impacts
|
||
|
|
|
||
|
|
The refactoring should have no external API impact. Performance impact should be
|
||
|
|
profiled.
|
||
|
|
|
||
|
|
## Testing
|
||
|
|
|
||
|
|
One of the goals is to have better unit test coverage. Overall functionality to
|
||
|
|
be tested through functional and regression tests.
|