1# Phosphor-hwmon refactoring
2
3Author: Kun Yi <kunyi!>
4
5Created: 2019/09/05
6
7## Problem Description
8
9Convoluted code in phosphor-hwmon is imparing our ability to add features
10and identify/fix bugs.
11
12## Background and References
13
14A few cases of smelly code or bad design decisions throughout the code:
15
16* Limited unit testing coverage. Currently the unit tests in the repo [1] mostly
17   test that the sensor can be constructed correctly, but there is no testing
18   of correct behavior on various sensor reading or failure conditions.
19*  Bloated mainloop. mainloop::read() sits at 146 lines [2] with complicated
20   conditions, macros, and try/catch blocks.
21*  Lack of abstraction and dependency injection. This makes the code hard to
22   test and hard to read. For example, the conditional at [3]
23   can be replaced by implementing different sensor::readValue() method for
24   different sensor types.
25
26[1](https://github.com/openbmc/phosphor-hwmon/tree/master/test)
27[2](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L390)
28[3](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L408)
29
30## Goals
31
321. Improve unit test coverage
332. Improve overall design using OOD principles
343. Improve error handling and resiliency
354. Improve configurability
36    *  By providing a common configuration class, it will be feasible to add
37       alternative configuration methods, e.g. JSON, while maintaining backward
38       compatibility with existing env files approach.
39
40## Proposed Design
41
42Rough breakdown of refactoring steps:
43
441. Sensor configuration
45    *  Add a SensorConfig struct that does all std::getenv() calls in one place
46    *  DI: make the sensor struct take SensorConfig as dependency
47    *  Unit tests for SensorConfig
48    *  Add unit tests for sensor creation with various SensorConfigs
492. Abstract sensors
50    *  Refine the sensor object interface and make it abstract
51    *  Define sensor types that inherit from the common interface
52    *  Add unit tests for sensor interface
53    *  DI: make the sensor map take sensor interface as dependency
54    *  Add a fake sensor that can exhibit controllable behavior
553. Refactor sensor management logic
56    *  Refactor sensor map to allow easier lookup/insertion
57    *  Add unit tests for sensor map
58    *  DI: make all other functions take sensor map as dependency
594. Abstract error handling
60    *  Add overridable error handler to sensor interface
61    *  Define different error handlers
62    *  Add unit tests for error handlers using the fake sensor
63
64## Alternatives Considered
65N/A.
66
67## Impacts
68The refactoring should have no external API impact. Performance impact should
69be profiled.
70
71## Testing
72One of the goals is to have better unit test coverage. Overall functionality
73to be tested through functional and regression tests.
74