intelligent-agent / redeem

Firmware for Replicape
http://wiki.thing-printer.com/index.php?title=Redeem
GNU General Public License v3.0
36 stars 44 forks source link

Configuration #144

Closed Wackerbarth closed 5 years ago

Wackerbarth commented 5 years ago

This is a part of the effort to separate aspects of the printer from characteristics of the object that stores the configuration.

The aspects of the printer are "values" stored in a "database". They are separate from the characteristics of the storage of the database.

ThatWileyGuy commented 5 years ago

Firstly, this breaks the M19 unittest.

More generally, though, I'm not sure what the end goal of these changes is. So far, it looks like you're moving Replicape/Reach versions from something that's detected and configured at runtime to something that's specified in the user configuration. I don't really see how that's an improvement - the available hardware is something we can probe at runtime and discover perfectly every time, so moving that information into the config just seems like a needless opportunity for a user to mess things up?

Wackerbarth commented 5 years ago

Actually, there is one additional commit which factors all of the hardware related items out and places them in a "Hardware.py" module. I'll add it in when rebase this to follow after #143 is merged.

I'm not sure that this is what's breaking your M19 test. In commit a89c5ff, you changed from "B3" steppers to "A4" steppers. However, there are no "A4" steppers in the default configuration.

Wackerbarth commented 5 years ago

In addition to making the configuration just a data store and handling the controller/hardware functions in separate modules, the idea is to examine the hardware and post the result in the information database. Then the configuration can be built from those values. Among other aspects, this will make it easier for us to test portions of the system without requiring the actual hardware.

ThatWileyGuy commented 5 years ago

I don't particularly like dynamically building up an object to represent the printer as the code you're rewriting did, but the one thing it did very well was make it clear that configuration is a mutable user-modifiable set of values loaded from a file, whereas hardware is an immutable set of values determined by what devices/capes/expansion boards we detect at startup. Putting the detected hardware at the same level as the user configuration loses that separation for seemingly very little benefit.

You commented before that you had another commit you wanted to add that you thought would make the distinction between hardware and configuration more clear - is that still coming?

Wackerbarth commented 5 years ago

For the current release cycle, this is all of the change that I am trying to get into the code. The creation of the "Hardware.py" module is intended to extract the part that is truly hardware specific from the general operation. We could create an additional data store, but I see little gain in doing so. One purpose in separating the identification of the hardware from the construction of the objects that will perform the functional implementation is that we are able to detail the specifics of each board revision in a data file rather than in more difficult to edit and runtime footprint increasing python code. These data files can be processed by the already present data store mechanism. It is my intention to remove the system provided "configuration files" from the user area and access them directly from the programs file space. To the extent permitted, the user will still be able to provide override settings in their "local" file.

ThatWileyGuy commented 5 years ago

This is a sweeping refactor that we never reached consensus on and are now unlikely to.