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.

Sign up for GitHub

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

Jump to bottom

feature(api): Allow simulator to load multiple of a module #14628

Merged
merged 31 commits into from
Mar 21, 2024
Merged

feature(api): Allow simulator to load multiple of a module #14628

merged 31 commits into from
Mar 21, 2024

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Mar 11, 2024 •
edited
Loading

Overview

closes https://opentrons.atlassian.net/browse/RSS-497.
extend simulator attached modules to allow multiples of a module.

Test Plan

tested with dev server on OT2 and Flex.

GET /modules and make sure you are able to see multiple item of the same module (test.json and test-flex.json)

Review requests

Should only affect the simulator but lets make sure?
Should I add more tests?

Risk assessment

low. should only affect simulators.

@codecov Codecov
Copy link

codecov bot commented Mar 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.74%. Comparing base (795bda5) to head (880c86f).
Report is 164 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14628      +/-   ##
==========================================
- Coverage   67.75%   67.74%   -0.01%     
==========================================
  Files        2521     2521              
  Lines       72207    72179      -28     
  Branches     9293     9293              
==========================================
- Hits        48922    48899      -23     
+ Misses      21064    21059       -5     
  Partials     2221     2221
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...i/src/opentrons/drivers/heater_shaker/simulator.py 90.00% <ø> (-0.48%) ⬇️
api/src/opentrons/drivers/mag_deck/simulator.py 88.00% <ø> (-0.89%) ⬇️
...pi/src/opentrons/drivers/rpi_drivers/interfaces.py 75.00% <ø> (-8.34%) ⬇️
api/src/opentrons/drivers/rpi_drivers/usb.py 75.00% <ø> (-1.00%) ⬇️
...src/opentrons/drivers/rpi_drivers/usb_simulator.py 100.00% <ø> (ø)
api/src/opentrons/drivers/temp_deck/simulator.py 92.00% <ø> (-0.31%) ⬇️
...pi/src/opentrons/drivers/thermocycler/simulator.py 91.83% <ø> (-0.17%) ⬇️
api/src/opentrons/hardware_control/api.py 82.53% <ø> (+0.19%) ⬆️
...pentrons/hardware_control/backends/ot3simulator.py 86.23% <ø> (ø)
...c/opentrons/hardware_control/backends/simulator.py 88.18% <ø> (-0.19%) ⬇️
... and 11 more

sfoster1
sfoster1 reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks great, maybe just make it sim_serial_number just to be super clear

api/src/opentrons/hardware_control/module_control.py Outdated Show resolved Hide resolved
api/src/opentrons/hardware_control/modules/utils.py Outdated Show resolved Hide resolved
api/src/opentrons/hardware_control/modules/heater_shaker.py Outdated Show resolved Hide resolved
sfoster1
sfoster1 requested changes Mar 19, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Couple things to fix about the way the module calls work but otherwise looking pretty good!

api/src/opentrons/hardware_control/simulator_setup.py Show resolved Hide resolved
robot-server/simulators/test-flex.json Outdated Show resolved Hide resolved
@TamarZanzouri TamarZanzouri changed the title Rss 497 extend test simulator to allow a mo m feature(api): Allow simulator to load multiple of a module Mar 21, 2024
@TamarZanzouri TamarZanzouri marked this pull request as ready for review March 21, 2024 02:10
@TamarZanzouri TamarZanzouri requested review from a team as code owners March 21, 2024 02:10
sfoster1
sfoster1 approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@sfoster1 sfoster1 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, nice change!

@TamarZanzouri TamarZanzouri merged commit 3aa46e0 into edge Mar 21, 2024
24 checks passed
@TamarZanzouri TamarZanzouri deleted the RSS-497-extend-test-simulator-to-allow-a-mo-m branch March 21, 2024 16:44
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
@TamarZanzouri @Carlos-fernandez
feature(api): Allow simulator to load multiple of a module (#14628)
17dfd79 
# Overview

closes https://opentrons.atlassian.net/browse/RSS-497.
extend simulator attached modules to allow multiples of a module.

# Test Plan

tested with dev server on OT2 and Flex. 

GET `/modules` and make sure you are able to see multiple item of the
same module (`test.json` and `test-flex.json`)

# Review requests

Should only affect the simulator but lets make sure?
Should I add more tests?

# Risk assessment

low. should only affect simulators.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfoster1 sfoster1 sfoster1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TamarZanzouri @sfoster1