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

Refactor using DriverModel #3

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Refactor using DriverModel #3

wants to merge 22 commits into from

Conversation

PeterKraus
Copy link
Contributor

@PeterKraus PeterKraus commented Aug 22, 2024

This PR will refactor @AlexN7-Shepard's implementation using tomato's DriverModel.

Tasks:

  • connecting to a flow controller
  • connecting to a pressure controller
  • determining capabilities and units from the device
  • reading properties from a flow controller
  • reading properties from a pressure controller
  • setting properties
  • implementing Tasks
  • testing functionality on hardware using tomato
  • implementing internal tests

src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
@PeterKraus
Copy link
Contributor Author

@AlexN7-Shepard It looks like you've added a new file DriverModel.py instead of merging the changes into the __init__.py of the package. The code will have to be merged and the DriverModel.py file deleted.

Copy link
Contributor Author

@PeterKraus PeterKraus left a comment

Choose a reason for hiding this comment

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

I don't think your code works. Have you tried it with the hardware? There are a lot of extra functions that don't need to be there and a lot of function that have to be there for the class to instantiate are missing. What's going on?

Comment on lines 7 to 10
# you mentionned that the property map should be outside, check
# but also that the parameters were hard coded, for more I think you asked to handle this :
# here is a proposition for more flexibility :
class PropertyMap:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the benefit of having this as a Class, since there are no useful class methods in it. I'm not sure we'll ever need to add a property from within tomato...

src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
Comment on lines 127 to 128
else:
raise ValueError(f"Unknown device type: {device_type}. Expected 90 for MFC or 91 for PC.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Syntax error due to bad indent here.

src/tomato_bronkhorst/__init__.py Outdated Show resolved Hide resolved
)
return attrs_dict

def _capabilities(self, **kwargs) -> set:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should also be on the DeviceManager, and capabilities not _capabilities.

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.

2 participants