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

Create simple box device for memory input/output #70

Merged
merged 4 commits into from
Jun 15, 2023
Merged

Conversation

callumforrester
Copy link
Contributor

This PR just illustrates an idea that came up in discussion with @abbiemery, not necessarily to be merged.

The envisioned use of this class is where you wish to simulate a network interface but the internal hardware logic is either not needed or very simple. A custom adapter can be made for the network interface and can simply read and write to an IoBox. I wondered if a lot of people would write devices like this and if it would make sense for tickit to provide a simple helper device.

Any thoughts welcome.

@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #70 (448ce61) into master (ada93e7) will increase coverage by 0.14%.
The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   92.32%   92.47%   +0.14%     
==========================================
  Files          44       45       +1     
  Lines        1186     1222      +36     
==========================================
+ Hits         1095     1130      +35     
- Misses         91       92       +1     
Impacted Files Coverage Δ
src/tickit/devices/iobox.py 97.22% <97.22%> (ø)

@abbiemery
Copy link
Collaborator

For context my use case is for creating devices/etc to be used in system tests in Artemis (ophyd-bluesky). They just need to be set and read from a network and do not need to talk to each other. I'm sure I'll see soon enough if I end up making a lot of these.

Copy link
Member

@garryod garryod left a comment

Choose a reason for hiding this comment

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

This seems like a really nice helper, I reckon it's a good addition to the framework.

It may be useful to add the option of outputting select keys at update time, thoughts?

tickit/devices/iobox.py Outdated Show resolved Hide resolved
@callumforrester
Copy link
Contributor Author

callumforrester commented Jul 22, 2022

I considered outputting keys at update time, what do you think the best solution is out of these?

  1. Have a non-total TypedDict with untyped fields
  2. Have a single field whose value is a dictionary/dataclass
  3. Write fields you wish to output manually in Inputs of subclasses

Or indeed, do you have any other ideas?

@garryod
Copy link
Member

garryod commented Jul 25, 2022

I considered outputting keys at update time, what do you think the best solution is out of these?

  1. Have a non-total TypedDict with untyped fields
  2. Have a single field whose value is a dictionary/dataclass
  3. Write fields you wish to output manually in Inputs of subclasses

Or indeed, do you have any other ideas?

A partial TypedDict with untyped fields seems the best without having the device grow arms and legs. Ultimately we accept that we sacrifice static type checking by using a device like this. It may however be worth considering adding run-time type checking if the user enters a mapping of outputs and their type in the config yaml, though with how easy it is to write a custom device this seems unlikely to be necessary, especially considering the opacity of such an implementation

Copy link
Member

@garryod garryod left a comment

Choose a reason for hiding this comment

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

Would be good to include a system test to cover the ComponentConfig

Copy link
Collaborator

@abbiemery abbiemery left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@callumforrester callumforrester merged commit cd428de into master Jun 15, 2023
@callumforrester callumforrester deleted the box-device branch June 15, 2023 12:20
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