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

Identifying juami #133

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

Identifying juami #133

wants to merge 23 commits into from

Conversation

dimacio
Copy link

@dimacio dimacio commented Dec 18, 2019

I had to change _load_arduino () to recognize my port in my Linux machine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it allows you to run the config file after telling you that the board is not a pytentiostat.

…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it print that the pytenstiostat is connected to the port after telling you that the board is not a pytentiostat.
…inux machine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it print that the pytenstiostat is connected to the port after telling you that the board is not a pytentiostat."

This reverts commit 0cca2c8.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it allows you to run the config file after telling you that the board is not a pytentiostat.
Copy link
Member

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

nice changes @dimacio thanks so much.

Before merging we need to guard against unexpected behavior and make sure this doesn't break the operations of the JUAMI potentiostats. @dimacio Please could you try and write some tests that try and break this and make sure it doesn't break. Do you know how to write tests? We are using pytest. @aplymill7 @jlhitt1993 @mspence2 , please could one of you try an run one of our JUAMI built potentiostats from this code and make sure it works?

Comment on lines 26 to 28
if re.match(r'tty|Arduino|COM',p.description) is not None:
com = p.device
n_arduinos += 1
Copy link
Member

Choose a reason for hiding this comment

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

I believe this gives false positives of Arduinos connected if this regular expression matches anythin with tty OR Arduino OR COM. Unless I'm reading the expression incorrectly, can we make this more specific so it more precisely identifies if it is an Arduino that is connected?

Copy link
Author

Choose a reason for hiding this comment

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

This was done to solve my problem to recognize the board in linux. The identification was performed in the 46 and 47

Copy link
Member

Choose a reason for hiding this comment

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

We should move that part into load_arduino (right after this) then cause we iterate number of arduinos here.

Copy link
Author

Choose a reason for hiding this comment

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

I have done it to recognize my port

@@ -38,14 +38,16 @@ def _initialize_arduino(com):
Creates board object with Arduino(). If the connection fails it prints an error message and exits.
Parameters
----------
com: string
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this part of doc string is deleted.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, that was an error.

Copy link
Author

Choose a reason for hiding this comment

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

that was a mistake

Comment on lines 47 to 50
print("Pytentiostat connected to {}.\n".format(
if board.firmware == 'pytentiostat_firmata.ino':
print("Pytentiostat connected to {}.\n".format(
com))
else:
print('the board is not a Pytentiostat')
Copy link
Member

Choose a reason for hiding this comment

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

Ah okay this is good way to check if it is a pytentiostat.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is the true change.

Copy link
Collaborator

@jlhitt1993 jlhitt1993 Dec 18, 2019

Choose a reason for hiding this comment

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

This is a good addition 👍 Right now you are working on the code that runs entirely in the command line but we have another version in the GUI folder that has a GUI and works differently. I think it would be great if you could implement this functionality there as well.

@aplymill7
Copy link
Member

Appears to work on my end with some brief testing, but a few more comments:

  1. Not sure why you needed to change the firmware, as it works on our end. On your potentiostat, did you build it to write from pin9?

  2. I do think it's a bit confusing if it lets you keep running after telling you wrong firmware is uploaded, so I think this check should be moved into the load_arduino function.

  3. The reason this PR is failing checks is because it cannot pass the test_routines (pytentiostat/test/test_routines.py). In there you should modify the test for the new way you are changing port recognition.

@sbillinge
Copy link
Member

@dimacio this seems to be failing tests.

@dimacio
Copy link
Author

dimacio commented Dec 18, 2019

Thanks @aplymill7 for the feedback. I will pass the code to load_arduino () and study pytest to adapt the checkpoint to this new condition.

@aplymill7 aplymill7 added the enhancement New feature or request label Dec 18, 2019
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it print that the pytenstiostat is connected to the port after telling you that the board is not a pytentiostat.
…inux machine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it print that the pytenstiostat is connected to the port after telling you that the board is not a pytentiostat."

This reverts commit 0cca2c8.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it allows you to run the config file after telling you that the board is not a pytentiostat.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…ing_juami

# Conflicts:
#	pytentiostat/routines.py
@codecov-io
Copy link

codecov-io commented Jan 8, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@8e370fa). Click here to learn what that means.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #133   +/-   ##
=========================================
  Coverage          ?   73.22%           
=========================================
  Files             ?        8           
  Lines             ?      295           
  Branches          ?        0           
=========================================
  Hits              ?      216           
  Misses            ?       79           
  Partials          ?        0
Impacted Files Coverage Δ
pytentiostat/routines.py 67.39% <88.88%> (ø)
tests/test_routines.py 97.61% <90.9%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e370fa...acd1b33. Read the comment docs.

@dimacio
Copy link
Author

dimacio commented Jan 8, 2020

I tried to correct the original code to end the execution when the potentiate is not recognized and adapt the tests to recognize this change. It worked fine on my machine, but when I uploaded the changes I had an error with codecov. Could you please tell me how to solve it?

@aplymill7
Copy link
Member

@dimacio I haven't seen this error before, but maybe linked with losing track of base commit (https://docs.codecov.io/docs/error-reference#section-missing-base-commit)? Maybe could be problem when you merged another branch onto the pushed branch? Could perhaps revert that commit, and then try to implement the changes on the fork with the base commit directly?

Copy link
Member

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

@dimacio it looks great to me, thanks! I can't test the functionality but I made a couple of comments.

@@ -38,7 +38,7 @@ def test_load_arduino():
def test_initialize_arduino():
da = Dummy_arduino()
da.name = "good_arduino"

da.firmware = "pytentiostat_firmata.ino"
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to be tested. We would need some assert statements that test different scenarios.

Copy link
Author

Choose a reason for hiding this comment

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

I pushed a new version with an assert statement to test. I think that the negative check has been already covered in line 42 of test_routines.py, I'm wrong?

Copy link
Member

Choose a reason for hiding this comment

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

you are testing desired behavior of the app. So answer these questions:
What do you want the program to do if da.firmware is the wrong value?
or None?
or even the right value? Right now we are not really testing anything because we set an attribute to something then ask if it is that thing.

Copy link
Author

Choose a reason for hiding this comment

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

Now I think I got it. Create two different instances, one that has the correct firmware and one that doesn't. The good arduino should return his name and the bad one should raise an error.

except:
sys.exit("Error. Could not open COM port")

if board.firmware == 'pytentiostat_firmata.ino': # check if the board have the right firmware
print("Pytentiostat connected to {}.\n".format(
Copy link
Member

Choose a reason for hiding this comment

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

why do you end this with a line-break? In general we like to keep any output compact (and minimal). Dislike chatty programs. Prefer better designed internals....

Copy link
Author

Choose a reason for hiding this comment

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

sorry, it was a typing mistake

Copy link
Member

Choose a reason for hiding this comment

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

👍 no need to apologize....

@dimacio dimacio closed this Jan 8, 2020
@dimacio dimacio deleted the identifying_juami branch January 8, 2020 18:49
@dimacio dimacio restored the identifying_juami branch January 8, 2020 18:59
@dimacio dimacio reopened this Jan 8, 2020
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
@dimacio
Copy link
Author

dimacio commented Jan 8, 2020

@aplymill7 I think I could solve the problem by returning to an earlier version of the master branch.

…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
@sbillinge
Copy link
Member

@dimacio please can you make your commit messages meaningful? It is important for them to describe what is in the commit. To understand why, type git log and try and find a commit where you did a specific thing...or worse, try and do that from someone else's commit history.... We don't really enforce it but it is sometimes useful to use the numpy system for commit messages. Look for "writing the commit message" here:
https://docs.scipy.org/doc/numpy-1.15.1/dev/gitwash/development_workflow.html

@dimacio
Copy link
Author

dimacio commented Jan 13, 2020

@sbillinge i changed the commit message according to the numpy workflow. Is clearly enough now?

@sbillinge
Copy link
Member

Thanks @dimacio that is much better. Please keep it up!

We should have a little discussion with @aplymill7 and others in the team about the tests. How do we want the program to behave under different scenarios of the board returning different things to ``initialize_arduino()". Once we have this sorted it will be very quick to write the tests and tweak the code the pass the tests and this can be merged.
@aplymill7 we want a list of things like

  1. no response from arduino
  2. somthing something, wrong firmware
  3. etc.

@sbillinge
Copy link
Member

@dimacio actually rereading the commit message it isn't quite right. First, it seems to be describing the topic of the whole PR and not the topic of that particular commit. Second, it is a bit general and not specific. what functionality? What is the issue with recognizing the potentiostat is it solving?

Don't bother changing old commit messages, it is not worth the time here, but I am just trying to nudge you towards the right direction in the workflow. Your contributions are really appreciated, this is not meant to be criticism, but it helps the project to have high quality code, and that includes documentation in the code and around the commits and the releases. Thanks so much for your efforts!

@aplymill7
Copy link
Member

Ah yes we can discuss this more. I'll look over the logic and start making a comprehensive list of things that can happen and what the expected response should be in each scenario.

@aplymill7
Copy link
Member

Here's what I came up with what should happen when executing startup routine, in which initialize_arduino is a part of.

  1. After ports listed that should correspond with our device are listed one of the following should happen:
    1a) If a single port is connected to an Arduino Uno and the port’s name is X, print “Arduino Uno found at port X”
    1b) If multiple ports are connected to Arduino Unos then print “More than one Arduino Uno found. Exiting…” and then system exit
    1c) If no port is found to be connected to an Arduino Uno then print “No Arduino Uno found. Exiting…” and then system exit.
  2. If starting the board object fails, then print “Error. Could not open port Arduino Uno is connected to. Exiting…” and then system exit.
  3. If the board doesn’t have the correct firmware then the system should prompt “Arduino Uno connected does not have correct firmware uploaded. Exiting…” and then system exit.
  4. If iterator fails, prompt “Iterator failed to start. Exiting…” and then system exit.
  5. If pin assignment fails, prompt “Failure to assign pins. Exiting…” and then system exit.

If you think there should be edits please let me know!

@sbillinge
Copy link
Member

sbillinge commented Jan 16, 2020 via email

…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. I had to remove the declaration of the pin 9 as out in the setup the pytentiostat_firmata.ino to use the board.firmware attribute because with the original it returns None. I don't know if it's a little confusing the result because if the wrong firmata was uploaded into the board it allows you to run the config file after telling you that the board is not a pytentiostat.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
…hine. Then I add a new checkpoint to verify that the firmware correspond to the one we want. At last I change the test_routines.py to recognized the firmware.
Messages changed to recognize the possible problems in the startup routine.
@dimacio
Copy link
Author

dimacio commented Jan 22, 2020

@aplymill7 I changed the messages according to your suggestion, I didn't do the points 4 and 5 because i want to finished de identifying problem before further changes to keep the branch as granular as possible. I should adjust the test to this new version of the routines?

@dimacio
Copy link
Author

dimacio commented Jan 22, 2020

@sbillinge The commit messages are more appropriate this time?

Copy link
Member

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks @dimacio the messages are much better. Please see the inline comments.

except:
sys.exit("Error. Could not open COM port")
sys.exit("Error: Could not open port Arduino Uno is connected to. Exiting…")
Copy link
Member

Choose a reason for hiding this comment

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

ERROR

sys.exit("Error: Could not open port Arduino Uno is connected to. Exiting…")

if board.firmware == 'pytentiostat_firmata.ino': # check if the board have the right firmware
print("INFO: Pytentiostat connected to {}.\n".format(com))
Copy link
Member

Choose a reason for hiding this comment

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

pls remove line-break

Messages changed to standardize the alerts.
Copy link
Member

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

Thanks @dimacio

The last thing that I think is needed is some more testing. You have made a nice mocked good and bad arduino, but you are not really testing that the code behaves the way you want it to when it retrieves the bad arduino. Also, there is no testing of the regular expression etc. A few more asserts would be great before we merge.

Thanks so much for htis.

Comment on lines 26 to 29
if re.search("tty|Arduino Uno|COM",p.description) is not None:
com = p.device
n_arduinos += 1
print("INFO: Arduino Uno found at port.\n".format(com))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This increases n_arduinos by 1 for every COM device connected to the computer. It needs to be specific for finding arduinos only. Could you tell us what string does your system return for p.description when p is the arduino? Maybe we can help here.

Copy link
Author

Choose a reason for hiding this comment

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

Yes of course, the system return "ttyACM0". I guess that is because I'm using a Linux machine.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should test the scenario if one of the USBs has a device connected at the same time as an arduino, especially if the port would be listed before the one at the arduino. For instance if we want to use the check description case if opening serial connection or check description fails, should try another port that matched the search. Maybe search should make a list of possibly compatible devices.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After looking on google, it looks like the Arduino Uno description will usually show up in linux as "ttyACMX". On windows it shows up as "Arduino Uno (COMX)". Maybe the best thing to do here is...
if re.search("ttyAMC|Arduino Uno", p.description) is not None: ...

Copy link
Member

Choose a reason for hiding this comment

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

I guess last one is mac case?

sys.exit("Error. Could not open COM port")
sys.exit("ERROR: Could not open port Arduino Uno is connected to. Exiting…")

if board.firmware == 'pytentiostat_firmata.ino': # check if the board have the right firmware
Copy link
Collaborator

Choose a reason for hiding this comment

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

On my system, board.firmware returns "pytent_firm.ino". Not sure why it's different.

Copy link
Author

Choose a reason for hiding this comment

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

I don't sure but maybe you could have changed the name of the firmware uploaded in the arduino? I used the one uploaded here https://github.com/juami/potentiostat_firmata.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see. The name must have changed when we made that repository. I will update mine.

@sbillinge
Copy link
Member

sbillinge commented Feb 1, 2020 via email

@aplymill7
Copy link
Member

Yeah that's fine if we want to support one device on one USB. I just wanted to not limit if a user had something else plugged into another port (i. e. not that our program has to work simultaneously with that port, but would it stop one device one usb from working). Like how would it handle if a flash drive is in a usb, or they have some other device connected that might give ours a false positive, but crash when trying to run?

@jlhitt1993
Copy link
Collaborator

@aplymill7 For the second part, I think that the firmware check will be pretty robust. It should only fail if someone has another Arduino Uno with the same firmware plugged in. As far as our program messing with other USB devices, it seems like serial.tools.list_ports.comports() is to blame. Not sure if we can fix that. I have never had any major issues with it though. It only seems to interrupt my bluetooth headphones for about a tenth of a second.

@aplymill7
Copy link
Member

Yeah maybe not a problem, I just haven't robustly checked. I know we catch later down the line with firmware check, but if a correct device is still possibly connected, it would be nice if the program connected to the correct device instead of hard exiting after the first device fails the firmware check.

@jlhitt1993
Copy link
Collaborator

Right. Right now it is only allowing the user to continue if one and only one Arduino Uno is connected. Seems safe for now but you can create an issue to add functionality to accommodate multiple Arduino Unos in the future.

@sbillinge
Copy link
Member

sbillinge commented Feb 1, 2020 via email

@dimacio
Copy link
Author

dimacio commented Feb 2, 2020

Sorry but it is not clear to me how I should continue. The boot version is ok? Should I modify it to restrict the strings accepted in the regular expression? Or should I proceed to develop more accurate tests to control the cases proposed so far?

@sbillinge
Copy link
Member

sbillinge commented Feb 2, 2020 via email

@jlhitt1993
Copy link
Collaborator

I agree with supporting the minimum for now. I would suggest changing line 26 based on the comment I left above and if more than one Arduino Uno is detected, then just let the script print "ERROR: More than one Arduino Uno found. Please unplug any other Arduinos and retry." then exit.

@aplymill7
Copy link
Member

Yeah support for minimum seems fine.

Modify the regular expression to recognize only the desired ports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants