-
Notifications
You must be signed in to change notification settings - Fork 44
handle multiple config versions #29
base: 2_2_x
Are you sure you want to change the base?
handle multiple config versions #29
Conversation
…aults are now in python) and modifying Redeem to instantiate using config factory
…onversion. updated Redeem class to handle config factories
Still working on getting the existing test suite to pass. Assuming there will be feedback so wanted to get the discussion going early. |
…o RedeemConfig accessors. porting get_key from cascadingparser into RedeemConfig
…to ajmirsky/config_versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally approved, but needs a rebase first. My only comments are minor nitpicks.
if config_parser.has_option('System', 'version'): | ||
version = config_parser.get('System', 'version') | ||
|
||
factory = ConfigFactoryV19() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine as-is, but in the long run, I think the factories need to be incremental and chainable. By that I meant that when we make more changes for 2.1, we don't want to have to go back and write those changes into the ConfigFactoryV19. If a user upgrades two versions, we really just want to use the ConfigFactoryV19 for the 1.9 -> 2.0 conversion and then write all the needed changes for 2.1 into ConfigFactoryV20.
This obviously isn't an immediate concern, but I think getting that sort of incremental model isn't possible the way the factories are currently written?
tests/core/test_configs.py
Outdated
self.assertAlmostEqual(redeem_config.getfloat('delta', 'c_angular'), 0.0) | ||
|
||
def test_old_config_into_new(self): | ||
"""test to make sure delta corrections are zero when tangential and angular are zero""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this test covers non-zero corrections?
tests/gcode/MockPrinter.py
Outdated
@@ -14,6 +14,7 @@ | |||
sys.modules['GPIO'] = mock.Mock() | |||
sys.modules['DAC'] = mock.Mock() | |||
sys.modules['ShiftRegister.py'] = mock.Mock() | |||
sys.modules['Adafruit_I2C'] = mock.Mock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this is actually unrelated and should disappear after a rebase?
…onfig factory will now inherit future changes. previous merge broke test suite, now fixed
With redeem constantly evolving, configuration files are changing such that old ones could break redeem. This pull request enables redeem to read in (and translate) older configuration files into whatever redeem is using as the latest configuration set. Implemented per the proposal:
https://docs.google.com/presentation/d/12VBJyAf_-Wa2F0C4hafdiJfBIZr64CXT95xBcniJcBU/edit?usp=sharing
By defining a "canonical" set of configuration settings, there are always known defaults. This means that a config file with incorrect syntax won't prevent redeem from starting.
The strategy for converting delta parameters was to first calculate the column positions using the equations on master based on tangential/radial. Then solving for the angular/radial components using the latest algorithm.