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 and
10identify/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 of
18  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 test
22  and hard to read. For example, the conditional at [3] can be replaced by
23  implementing different sensor::readValue() method for different sensor types.
24
25[1](https://github.com/openbmc/phosphor-hwmon/tree/master/test)
26[2](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L390)
27[3](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L408)
28
29## Goals
30
311. Improve unit test coverage
322. Improve overall design using OOD principles
333. Improve error handling and resiliency
344. Improve configurability
35   - By providing a common configuration class, it will be feasible to add
36     alternative configuration methods, e.g. JSON, while maintaining backward
37     compatibility with existing env files approach.
38
39## Proposed Design
40
41Rough breakdown of refactoring steps:
42
431. Sensor configuration
44   - Add a SensorConfig struct that does all std::getenv() calls in one place
45   - DI: make the sensor struct take SensorConfig as dependency
46   - Unit tests for SensorConfig
47   - Add unit tests for sensor creation with various SensorConfigs
482. Abstract sensors
49   - Refine the sensor object interface and make it abstract
50   - Define sensor types that inherit from the common interface
51   - Add unit tests for sensor interface
52   - DI: make the sensor map take sensor interface as dependency
53   - Add a fake sensor that can exhibit controllable behavior
543. Refactor sensor management logic
55   - Refactor sensor map to allow easier lookup/insertion
56   - Add unit tests for sensor map
57   - DI: make all other functions take sensor map as dependency
584. Abstract error handling
59   - Add overridable error handler to sensor interface
60   - Define different error handlers
61   - Add unit tests for error handlers using the fake sensor
62
63## Alternatives Considered
64
65N/A.
66
67## Impacts
68
69The refactoring should have no external API impact. Performance impact should be
70profiled.
71
72## Testing
73
74One of the goals is to have better unit test coverage. Overall functionality to
75be tested through functional and regression tests.
76