feat(api): register module instances on os events #4441
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b-cooper
added
feature
Previously, the hardware controller only became aware of underlying connected hardware modules when it was told to look via an http endpoint (only effectively used by the run app). In order to remove this unnecessary reliance, the hardware controller now watches the filesystem for changes using aionotify (a thin python wrapper of linux util). Changes to the physically connected modules should now be instantly reflected in the hardware controllers attached modules regardless of whether the hardware controller is observed by a third party or not. No more Schrödinger's modules re #3580
b-cooper
force-pushed
the
api_watch-modules
branch
from
b3b55b3 to
95d3559
Compare
sfoster1
requested changes
Nov 13, 2019
There was a problem hiding this comment.
Inline comments. Also,
- We need to specify the new
aionotifydependency in our pipfile specs (for devs) and in ourinstall_requiresin setup.py for pypi, presumably gated around linux - Bit concerned about the flow around updating modules when they switch ports, but I haven't tested yet
- We also need to test just what happens when you create a hardware controller on windows, since manufacturing does this all the time (or will): it should run, although it's ok if it's degraded
This was referenced Nov 15, 2019
Codecov Report
@@ Coverage Diff @@
## edge #4441 +/- ##
==========================================
+ Coverage 55.47% 56.62% +1.14%
==========================================
Files 904 906 +2
Lines 26119 28415 +2296
==========================================
+ Hits 14490 16089 +1599
- Misses 11629 12326 +697
Continue to review full report at Codecov.
|
sfoster1
reviewed
Nov 18, 2019
sfoster1
requested changes
Nov 18, 2019
| path='/dev', | ||
| flags=(aionotify.Flags.CREATE | aionotify.Flags.DELETE)) | ||
| except AttributeError: | ||
| MODULE_LOG.info( |
There was a problem hiding this comment.
Should definitely be warning level
| MODULE_LOG.info(f'Module Removed: {maybe_module_at_port}') | ||
| if maybe_module_at_port is not None and \ | ||
| aionotify.Flags.CREATE in flags: | ||
| await register_modules(new_modules=[maybe_module_at_port]) |
There was a problem hiding this comment.
Should we coalesce these? could this happen with
- events for more than one module
- create and then delete event for a module
- delete and then create event for a module
| @@ -201,3 +228,6 @@ def probe(self, axis: str, distance: float) -> Dict[str, float]: | |||
| self.pause() | |||
| await asyncio.sleep(duration_s) | |||
| self.resume() | |||
|
|
|||
| def __exit__(self, exc_type, exc_value, traceback): | |||
There was a problem hiding this comment.
I think this wants to be __del__ (https://docs.python.org/3/reference/datamodel.html#object.__del__), __exit__ (https://docs.python.org/3/reference/datamodel.html#object.__exit__) is for context managers, aka things you use in a with statement
b-cooper
force-pushed
the
api_watch-modules
branch
from
2a5633c to
1718908
Compare
sfoster1
reviewed
Nov 19, 2019
sfoster1
requested changes
Nov 19, 2019
| @@ -201,3 +234,6 @@ def probe(self, axis: str, distance: float) -> Dict[str, float]: | |||
| self.pause() | |||
| await asyncio.sleep(duration_s) | |||
| self.resume() | |||
|
|
|||
| def __del__(self, exc_type, exc_value, traceback): | |||
There was a problem hiding this comment.
Take a look at the docs again (https://docs.python.org/3/reference/datamodel.html#object.__del__), this can't take any arguments unfortunately
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
overview
Previously, the hardware controller only became aware of underlying connected hardware modules when
it was told to look via an http endpoint (only effectively used by the run app). In order to remove
this unnecessary reliance, the hardware controller now watches the filesystem for changes using
aionotify (a thin python wrapper of linux util). Changes to the physically connected modules should
now be instantly reflected in the hardware controllers attached modules regardless of whether the
hardware controller is observed by a third party or not. No more Schrödinger's modules
re #3580
changelog
hardware_controller.API.attached_modulesas a list ofAbstractModuleinstancesbuild_hardware_controllerare now skipped in caseaionotify/inotifyis not present (non-linux environments)review requests