Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jig mux alternate implementation #184

Merged
merged 42 commits into from
Jun 20, 2024

Conversation

clint-lawrence
Copy link
Collaborator

@clint-lawrence clint-lawrence commented Jun 3, 2024

This introduces a new implementation of the jig switching support code. The primary motivation/goals:

  • The previous implementation in jig_mapping had tight coupled between the address handlers and virtual mux. This made any changes or refactoring very difficult and made the code harder to understand than needed
  • The way JigDriver worked required AddressHandlers to be instantiated when a JigDriver subclass was defined. This more or less forced address handlers to reference a global object for any real IO.

This implementation make the "pin" concept the primary point of coupling between the AddressHandlers and VirtualMuxs. Then the JigDriver is being revised to improve type hinting of the jig driver object and also decouple instantiation at runtime from JigDriver subclass definition. In addition, I'm working towards the new code have complete (or near complete) type annotations.

While these changes are trying to avoid the need to change existing test scripts, it won't be possible to implement this in a completely backwards compatible way. So this will be introduced as a parallel implementation. Existing scripts will continue to run. New and updated scripts will import from a different submodule and be able to take advantage of the improved implementation. In time, the current jig_mapping implementation will be removed.

Once this work is done, it will be possible to move software with the related DriverManager change https://github.com/PyFixate/Fixate/pull/182/files

  • remove switch_through_all_signals. Instead, create a method that returns all signals on a mux.
  • modify iterate_all_mux_paths to return mux object and signal pairs inline with the change above to switch_through_all_signals
  • run on some hardware
  • write a migration guide for existing test scripts
  • write test for VirtualMux.multiplex
  • write test for VirtualAddressMap.add_update
  • Are there any additional cross-checks we should implement on virtualmux/addresshandler?
    As a minimum, add a method to address map that ensures all pins from all virtual muxes have pin defined in an address handler. Probably call this during the JigDriver init.
  • move imports to fixate.{VirtualMux, JigDriver, ...} etc?
  • Decide what to do with VirtualAddressMap.{update_pin_by_name,update_pins_by_name, __getitem__, __setitem__}
  • Should we allow overriding the default signal ""? In the previous jig_mapping implementation, the behaviour of the empty signal was hard coded. If you defined some pins against "", I'm pretty sure that would be ignored. Should we check that the user script doesn't redefine ""?
  • Do we need a MuxGroup.force_trigger or similar, that writes all the current mux states out to hardware, even if the pins haven't changed?
  • Do we need a MuxGroup.trigger() or VirtualAddressMap.trigger() that processes any pin changes in VirtualAddressMap._pending_updates (Issues created Add a force_trigger method to the jig driver #189 and Add a JigDriver.trigger() method #190)
  • Change JigDriver to accept a factory for the AddressHandlers? No, add open() method to address handlers that Jig Driver will call when it needs to. Make AddressHandlers accept a pin_list in init()
  • Add a "report" method to JigDriver that prints out all connected muxes and pins from the address handlers? (Issue created Create a "report" tool for a JigDrive #192)
  • Add an alias to RelayMatrixVirtualMux called BreakBeforeMakeMux. (Issue created Rename (or alias) RelayMatrixVirtualMux as BreakBeforeMakeMux #191)
  • Add a MakeBeforeBreakMux subclass of VirtualMux (Issue created Rename (or alias) RelayMatrixVirtualMux as BreakBeforeMakeMux #191)

@clint-lawrence
Copy link
Collaborator Author

I deleted a few comment and moved the content to #187

Copy link
Collaborator

@jcollins1983 jcollins1983 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of questions here and there.

Still not sure about this...
@clint-lawrence clint-lawrence marked this pull request as ready for review June 19, 2024 09:30
@clint-lawrence
Copy link
Collaborator Author

clint-lawrence commented Jun 19, 2024

O.K. this is no longer draft...

I'm still not 100% on the address handler stuff. But my concerns are really how that ties with #182, so I think I need to get this merged and then see where that PR ends up. If we end up reworking the address handlers in that PR that is O.K.

I've also created a handful of smaller issues. Once this PR is reviewed and merged we can decide if we want to do any of that work before making a new release.

Copy link
Collaborator

@jcollins1983 jcollins1983 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with where things are on this.

This silences a warning that were importing an private
module from fixate.core in the top level __init__.py. So
partly this is just to silence PyCharm, but it is a resonable
change in general.
@clint-lawrence clint-lawrence merged commit 5dcd515 into PyFixate:master Jun 20, 2024
9 checks passed
@clint-lawrence clint-lawrence deleted the jig-mux-refactor branch June 20, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants