Import driver import and other minor improvements

clint-lawrence commented 3 years ago

Less magic for driver imports
Don't create the sequencer object at import time. This helped fixed some circular imports
Removed some unused globals and commented out code
Remove some broken and unused functions from fixate.core.control

clint-lawrence commented 3 years ago

Probably just nitpicking on things but a few things I don't like about what is left. I guess for the point of discussion and practising code review it will still be useful.
def __init__(self, initial_state):
    self._set_point = 0.0
    try:
        self._updated_value = float(initial_state)
    except ValueError as e:
        raise ParameterError(
            "initial_state found {}. Must be a non-zero number".format(
                initial_state
            )
        ) from e
This try except doesn't actually catch when the initial state is zero
def set_point(self, value):
    self._set_point = value
How is this any more useful than setting the value directly? Should this be part of init? It looks like this was meant to be a standardised method between different controllers but then there is no base "controller" class that they all inherit from.

One thing I liked about converge_amplitude_scalar over converge_scalar was that it raised different exceptions for each reason, rather than just returning True if it succeeded or False if it failed. It makes it kind of annoying to have to open everything up in debug and manually step through the function to see what went wrong, especially because there is a timeout condition

Can also clean up unused imports.

All valid points. I was doing my best to not dig too deep in that corner. I was content to just delete stuff that is no longer being used. I think just deleting cruft is still valuable, because now there is much less distraction when we do come back around to try and improve things. Feel free to raise and issue or a new PR. I'm not planning anything further with this control.py module. Even getting some test coverage on this function would be good.

Possibly you'll have thoughts while continuing to troubleshoot the ELV issues.

clint-lawrence commented 3 years ago

I've now tested against all the driver. Import and get_identity() work as expected, with the exception of daq. The daq driver does import from the fixate POV, but the internal PyDAQmx library doesn't import because I don't have the drivers installed on my laptop. However, the aspects changed in this PR are sufficiently covered with that test.

PyFixate / Fixate

Import driver import and other minor improvements #133