1148272a0SKun Yi# Phosphor-hwmon refactoring 2148272a0SKun Yi 3148272a0SKun YiAuthor: Kun Yi <kunyi!> 4148272a0SKun Yi 5148272a0SKun YiCreated: 2019/09/05 6148272a0SKun Yi 7148272a0SKun Yi## Problem Description 8148272a0SKun Yi 9*f4febd00SPatrick WilliamsConvoluted code in phosphor-hwmon is imparing our ability to add features and 10*f4febd00SPatrick Williamsidentify/fix bugs. 11148272a0SKun Yi 12148272a0SKun Yi## Background and References 13148272a0SKun Yi 14148272a0SKun YiA few cases of smelly code or bad design decisions throughout the code: 15148272a0SKun Yi 16*f4febd00SPatrick Williams- Limited unit testing coverage. Currently the unit tests in the repo [1] mostly 17*f4febd00SPatrick Williams test that the sensor can be constructed correctly, but there is no testing of 18*f4febd00SPatrick Williams correct behavior on various sensor reading or failure conditions. 19*f4febd00SPatrick Williams- Bloated mainloop. mainloop::read() sits at 146 lines [2] with complicated 20148272a0SKun Yi conditions, macros, and try/catch blocks. 21*f4febd00SPatrick Williams- Lack of abstraction and dependency injection. This makes the code hard to test 22*f4febd00SPatrick Williams and hard to read. For example, the conditional at [3] can be replaced by 23*f4febd00SPatrick Williams implementing different sensor::readValue() method for different sensor types. 24148272a0SKun Yi 25148272a0SKun Yi[1](https://github.com/openbmc/phosphor-hwmon/tree/master/test) 26148272a0SKun Yi[2](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L390) 27148272a0SKun Yi[3](https://github.com/openbmc/phosphor-hwmon/blob/94a04c4e9162800af7b2823cd52292e3aa189dc3/mainloop.cpp#L408) 28148272a0SKun Yi 29148272a0SKun Yi## Goals 30148272a0SKun Yi 31148272a0SKun Yi1. Improve unit test coverage 32148272a0SKun Yi2. Improve overall design using OOD principles 33148272a0SKun Yi3. Improve error handling and resiliency 34148272a0SKun Yi4. Improve configurability 35*f4febd00SPatrick Williams - By providing a common configuration class, it will be feasible to add 36148272a0SKun Yi alternative configuration methods, e.g. JSON, while maintaining backward 37148272a0SKun Yi compatibility with existing env files approach. 38148272a0SKun Yi 39148272a0SKun Yi## Proposed Design 40148272a0SKun Yi 41148272a0SKun YiRough breakdown of refactoring steps: 42148272a0SKun Yi 43148272a0SKun Yi1. Sensor configuration 44*f4febd00SPatrick Williams - Add a SensorConfig struct that does all std::getenv() calls in one place 45*f4febd00SPatrick Williams - DI: make the sensor struct take SensorConfig as dependency 46*f4febd00SPatrick Williams - Unit tests for SensorConfig 47*f4febd00SPatrick Williams - Add unit tests for sensor creation with various SensorConfigs 48148272a0SKun Yi2. Abstract sensors 49*f4febd00SPatrick Williams - Refine the sensor object interface and make it abstract 50*f4febd00SPatrick Williams - Define sensor types that inherit from the common interface 51*f4febd00SPatrick Williams - Add unit tests for sensor interface 52*f4febd00SPatrick Williams - DI: make the sensor map take sensor interface as dependency 53*f4febd00SPatrick Williams - Add a fake sensor that can exhibit controllable behavior 54148272a0SKun Yi3. Refactor sensor management logic 55*f4febd00SPatrick Williams - Refactor sensor map to allow easier lookup/insertion 56*f4febd00SPatrick Williams - Add unit tests for sensor map 57*f4febd00SPatrick Williams - DI: make all other functions take sensor map as dependency 58148272a0SKun Yi4. Abstract error handling 59*f4febd00SPatrick Williams - Add overridable error handler to sensor interface 60*f4febd00SPatrick Williams - Define different error handlers 61*f4febd00SPatrick Williams - Add unit tests for error handlers using the fake sensor 62148272a0SKun Yi 63148272a0SKun Yi## Alternatives Considered 64*f4febd00SPatrick Williams 65148272a0SKun YiN/A. 66148272a0SKun Yi 67148272a0SKun Yi## Impacts 68*f4febd00SPatrick Williams 69*f4febd00SPatrick WilliamsThe refactoring should have no external API impact. Performance impact should be 70*f4febd00SPatrick Williamsprofiled. 71148272a0SKun Yi 72148272a0SKun Yi## Testing 73*f4febd00SPatrick Williams 74*f4febd00SPatrick WilliamsOne of the goals is to have better unit test coverage. Overall functionality to 75*f4febd00SPatrick Williamsbe tested through functional and regression tests. 76