-
Notifications
You must be signed in to change notification settings - Fork 179
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
Abr asair sensor #14445
Abr asair sensor #14445
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14445 +/- ##
==========================================
- Coverage 68.15% 68.14% -0.02%
==========================================
Files 1628 2518 +890
Lines 54954 72009 +17055
Branches 4141 9232 +5091
==========================================
+ Hits 37452 49067 +11615
- Misses 16814 20743 +3929
- Partials 688 2199 +1511
Flags with carried forward coverage won't be shown. Click here to find out more. |
) | ||
|
||
|
||
def _get_user_input(list: List, some_string: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list
is actually a predefined term in python - it is the name of the list type. defining a variable called list
will shadow the global name in the scope where the variable is defined, which would lead to surprising results if you wanted to build a list in this function. Would you mind calling this something else, like lst
or input_list
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to lst
) | ||
|
||
|
||
def _get_user_input(list: List, some_string: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelatedly, List
is a generic container type that is meant to be specialized with the type it contains - for instance, List[str]
is a list of strings, List[int]
is a list of ints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added List[str]
return variable | ||
|
||
|
||
class _abr_asair_sensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, common python style is for classes to be PascalCase
- in this case, _ABRAsairSensor
if the class isn't meant to be exported and ABRAsairSensor
if it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed class name _ABRAsairSensor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome. Just a few comments that shouldn't block merging
while True: | ||
env_data = sensor.get_reading() | ||
timestamp = datetime.datetime.now() | ||
new_timestamp = timestamp - datetime.timedelta(hours=5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this b/c of the timezone? either leaving a comment to describe what this is doing or somehow specifying the system's timezone would make it more clear
for sublist in results_list: | ||
row_str = ", ".join(map(str, sublist)) + "\n" # type: str | ||
result_string += row_str | ||
save_file_path = data.append_data_to_file( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you are appending the line to all previous lines, and then you appending that string to the file, and doing this for each line, which means that the file you are writing will be huge and filled with repeated lines.
This can be fixed by removing the result_string
variable and just passing the row_str
into append_data_to_file()
.
Then also, you can append each line to the CSV at the same time that you write the data to the google sheet. Just move the append_data_to_file()
up into while loop above. Then the CSV will be updated in realtime, plus you don't need to store data in the results_list
variable anymore.
<!-- Thanks for taking the time to open a pull request! Please make sure you've read the "Opening Pull Requests" section of our Contributing Guide: https://github.com/Opentrons/opentrons/blob/edge/CONTRIBUTING.md#opening-pull-requests To ensure your code is reviewed quickly and thoroughly, please fill out the sections below to the best of your ability! --> # Overview This PR adds a script to the hardware-testing folder that will allow the robot to connect to an asair temperature and humidity sensor and record continuously to a google spread sheet. # Test Plan I tested this script on robot DVT10 on software v7.1.1. It successfully measured the temperature and humidity and uploaded to the google sheet. # Changelog Added abr-asair-sensor script Script: 1. Asks user for robot name 2. Asks user for duration of recording (min) 3. Asks user for frequency of recording (min) 4. Robot reads ports and connects to asair sensor 5. robot collects data and appends to a list at the given frequency 6. At the end of the duration the robot saves a .csv file of the data to the testing-data folder within the robot and to the google sheet I also updated the analyze-abr script to improve ease of analysis. IMPORTANT: MAKE SURE ANY BREAKING CHANGES ARE PROPERLY COMMUNICATED --> # Review requests <!-- Describe any requests for your reviewers here. --> # Risk assessment - for the script to run successfully on the robot, google_sheets_tool.py and a google sheets credentials file needs to be uploaded manually onto the robot's jupyter notebook. - for google_sheets_tool.py to run successfully within the script python libraries: httplib2, oauth2client, gspread, and pprintpp need to be installed onto the robots
Overview
This PR adds a script to the hardware-testing folder that will allow the robot to connect to an asair temperature and humidity sensor and record continuously to a google spread sheet.
Test Plan
I tested this script on robot DVT10 on software v7.1.1. It successfully measured the temperature and humidity and uploaded to the google sheet.
Changelog
Added abr-asair-sensor script
Script:
I also updated the analyze-abr script to improve ease of analysis.
IMPORTANT: MAKE SURE ANY BREAKING CHANGES ARE PROPERLY COMMUNICATED
-->
Review requests
Risk assessment