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

draft #10000

Closed
wants to merge 4 commits into from
Closed

draft #10000

wants to merge 4 commits into from

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Apr 19, 2022

draft pr for sync binding function and tests

also number 10000 WOOOOO 🍾🍾🍾🍾
🎉🎉🎉 🎊🎊
🎉🎉🎉🎉🎊

@caila-marashaj caila-marashaj requested a review from a team as a code owner April 19, 2022 14:03
@caila-marashaj caila-marashaj marked this pull request as draft April 19, 2022 14:03
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #10000 (dc23391) into edge (ae7c625) will increase coverage by 0.01%.
The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #10000      +/-   ##
==========================================
+ Coverage   74.74%   74.75%   +0.01%     
==========================================
  Files        2110     2110              
  Lines       55429    55470      +41     
  Branches     5630     5630              
==========================================
+ Hits        41429    41466      +37     
- Misses      12866    12870       +4     
  Partials     1134     1134              
Flag Coverage Δ
app 71.62% <ø> (ø)
components 52.80% <ø> (ø)
hardware 63.28% <92.98%> (+0.34%) ⬆️
labware-library 49.74% <ø> (ø)
notify-server 89.17% <ø> (ø)
protocol-designer 44.88% <ø> (ø)

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

Impacted Files Coverage Δ
...ardware/opentrons_hardware/scripts/sensor_utils.py 0.00% <0.00%> (ø)
...ot-server/robot_server/protocols/protocol_store.py 94.73% <ø> (ø)
hardware/opentrons_hardware/sensors/sensor_abc.py 78.00% <80.00%> (-0.58%) ⬇️
hardware/opentrons_hardware/sensors/utils.py 92.45% <83.33%> (+0.78%) ⬆️
hardware/opentrons_hardware/sensors/fdc1004.py 100.00% <100.00%> (ø)
hardware/opentrons_hardware/sensors/mmr920C04.py 86.04% <100.00%> (+2.71%) ⬆️
hardware/opentrons_hardware/sensors/scheduler.py 91.66% <100.00%> (-2.34%) ⬇️
robot-server/robot_server/runs/run_store.py 100.00% <100.00%> (ø)
app/src/organisms/ProtocolDetails/index.tsx 54.05% <0.00%> (ø)
... and 4 more

@caila-marashaj caila-marashaj changed the title draft draft champagne Apr 19, 2022
@caila-marashaj caila-marashaj changed the title draft champagne draft Apr 19, 2022
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.

I think the structure is good, but theres some little things and you should refactor the parts that send the messages so it's harder to accidentally specify the wrong sensor

@@ -146,6 +146,9 @@ class MoveCompletedPayload(MoveGroupResponsePayload):
ack_id: utils.UInt8Field


# maybe write a set sync message
Copy link
Member

Choose a reason for hiding this comment

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

what would a set sync message be?

),
)

while True:
Copy link
Member

Choose a reason for hiding this comment

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

Definitely don't want to do this like this:

  1. shouldn't have a function that has an infinite loop with no break condition - there should always be something.
  2. lines 87 to 94 will lock this program - this is an async function and read() is a synchronous call, so the loop won't be able to spin.

)

while True:
read_data = self.read(
Copy link
Member

Choose a reason for hiding this comment

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

oh actually that second thing might just be that this needs to be an await self.read(... but the first thing is true

hardware/opentrons_hardware/sensors/mmr920C04.py Outdated Show resolved Hide resolved
node_id=node_id,
message=BindSensorOutputRequest(
payload=BindSensorOutputRequestPayload(
sensor=SensorTypeField(SensorType.capacitive),
Copy link
Member

Choose a reason for hiding this comment

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

pressure?

node_id=node_id,
message=BaselineSensorRequest(
payload=BaselineSensorRequestPayload(
sensor=SensorTypeField(SensorType.capacitive),
Copy link
Member

Choose a reason for hiding this comment

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

pressure

@classmethod
def build(cls, data): # type: ignore[no-untyped-def]
"""Build function for sensor data type."""
if isinstance(data, list):
backing = Int32Field(cls._convert_to_int(data))
elif isinstance(data, Int32Field):
# breakpoint()
Copy link
Member

Choose a reason for hiding this comment

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

delete

@caila-marashaj caila-marashaj deleted the RET-907-sensor-binding-drivers branch May 12, 2022 15:29
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.

None yet

2 participants