Short proposal on phosphor-hwmon refactoring
Signed-off-by: Kun Yi <kunyi731@gmail.com>
Change-Id: I3996ffe0b99c547312d46d05a4e7ec058f7b4695
diff --git a/designs/phosphor-hwmon-refactoring.md b/designs/phosphor-hwmon-refactoring.md
new file mode 100644
index 0000000..7a13087
--- /dev/null
+++ b/designs/phosphor-hwmon-refactoring.md
@@ -0,0 +1,73 @@
+# 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.