samiebee43
commented
10 months ago Yes this is kind of a lot to review at once, but most of it looks good.
Two things stand out to me.
1. Deprecation of plugin dependencies
Appears to be a duplicate of #311 . This can be handled a number of ways, but it may be worthwhile to diff the two branches to see if they differ more than expected. One way to do this would be.
# should contain the largest set of filenames
git diff --name-only mzieg-followup-testing 4.1.0-dev
# should contain a strict subset of filenames, plugin related
git diff --name-only mzieg-4.1.0-plugin-features 4.1.0-dev
# should contain a strict subset of filenames, not plugin related
git diff --name-only mzieg-followup-testing mzieg-4.1.0-plugin-featuresOverall, it's a non-issue. If both branches are good, then they can both be merged in any order and the whole thing should be fine, but it's a bit curious that such an overlap happened.
2. Preset Feature API
There's a nice comment that explains the preset feature's API.
class IntegrationTimeFeature:
def __init__(self):
ctl.presets.register(self, ["integration_time_ms"])
def get_preset(self, attr):
if attr == "integration_time_ms":
return self.current_ms
def set_preset(self, attr, value):
if attr == "integration_time_ms":
self.set_ms(int(value))This looks pretty good, it handles situations where we need to do more than update a variable--for example modifying the GUI to match. This is not the soln I had in mind, but that's okay. The main thing I would suggest is to avoid passing attr's, and to explicitly state callbacks in the registration step. Also, avoid passing an array to register--rather call it multiple times.
class IntegrationTimeFeature:
def __init__(self):
ctl.presets.register(self, "integration_time_ms", get_integration_time, set_integration_time_ms)
def get_integration_time_ms(self):
return self.current_ms
def set_integration_time_ms(self, value):
value = int(value)
# existing definition ... For completeness, the solution I had in mind would be to use resolution and search-paths. This would involve creating a parameter construct which represents the kinds of things that we might want to preset, persist, etc. Classes that depend on Enlighten Parameters access those values via a third party every time (kind of like how we access spec from multispec). Setting the value also uses the same third party. The parameter access routine will have some logic to decide whether to load the value from the .ini file, eeprom, a hardcoded default, or from the active preset. It requires that upon selecting a preset, that the GUI is updated to call in new resolutions for each parameter. Parameters perform validation and correction at access time.
class IntegrationTimeFearture:
def __init__(self):
pass
def get_integration_time_ms(self):
# update GUI if necessary
# ...
# get the parameter value, whether it's driven by config, preset, eeprom, etc
return EnlightenParameters.resolve("integration_time_ms")
def set_integration_time_ms(self, value):
# save the parameter value, storing changes in config or preset as appropriate.
EnlightenParameters.save("integration_time_ms", value)
# update GUI if necessary
# ...Upon enabling a preset, we would need to make sure IntegrationTimeFeature is updated by calling get_integration_time_ms to get the latest decision from EnlightenParameters.
mzieg
I probably should have broken this up into more PRs.