Identifying juami

[dimacio]

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.

[aplymill7]

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]

@dimacio this seems to be failing tests.

[dimacio]

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

[codecov-io]

Codecov Report

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

@@            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]

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]

@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?

[dimacio]

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

[sbillinge]

@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]

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

[sbillinge]

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]

@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]

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]

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]

just one suggestion to think about. When the program exits always put in the message ERROR: blah blah and when it doesn't exit put WARNING: blah blah. If it is just general chatter put INFO: blah blah

S

On Thu, Jan 16, 2020 at 1:49 PM aplymill7 notifications@github.com wrote:

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!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/133?email_source=notifications&email_token=ABAOWUKWT2SI5JXRTZH3BZTQ6CT4XA5CNFSM4J4MOPX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJFD7KQ#issuecomment-575291306, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUM24P2X53UMVX47XYDQ6CT4XANCNFSM4J4MOPXQ .

-- Professor Simon Billinge Columbia University

[dimacio]

@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]

@sbillinge The commit messages are more appropriate this time?

[sbillinge]

Before getting too deeply into this we might want to think about which use cases we want to support and behavior we want. For example, it’s OK to just support one device on one USB etc.

On Sat, Feb 1, 2020 at 3:23 PM aplymill7 notifications@github.com wrote:

@aplymill7 commented on this pull request.

In pytentiostat/routines.py:

• 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))

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/133?email_source=notifications&email_token=ABAOWUIGBQTXKKGBJ3WSZWLRAXK3XA5CNFSM4J4MOPX2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCT4ZYVY#discussion_r373800330, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUPAK3P2QZRAT2RHE5DRAXK3XANCNFSM4J4MOPXQ .

-- Professor Simon Billinge Columbia University

[aplymill7]

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]

@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]

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]

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]

This could be ok. Only worry about the more complicated UC if someone requests it....

On Sat, Feb 1, 2020 at 4:24 PM Jeremy Hitt notifications@github.com wrote:

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/133?email_source=notifications&email_token=ABAOWUN53ABDBYV7752XBI3RAXR7XA5CNFSM4J4MOPX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRG7SQ#issuecomment-581070794, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUND3M5Y4XMMULVLR4DRAXR7XANCNFSM4J4MOPXQ .

-- Professor Simon Billinge Columbia University

[dimacio]

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]

Hi @dimacio I think there is a discussion that needs to take place about how we want the software to behave before we know exactly what needs to be done next. When that conversation is converged then the path forward is to write tests that test for that behavior, then to write code until the tests pass.

For the discussion, my suggestion is to support less, then implement more, functionality when it is needed/asked for. i.e., implement the mvp (minimum viable product). I am not sure what that is here....just one serial device plugged in to a USB? Whatever we decide, we want the program to be more friendly to the user rather than less friendly, so if we decide the above, and the user tries to use it with two devices plugged in, our code could either just crash, or it could detect two devices and then gently tell the user that this is not supported yet and would they mind unplugging the non pytentiostat one, for example. On the whole it is often better to avoid the program doing magic. That always seems like a good idea, but quickly gets difficult to debug. These are just general comments. I think Austin and Jeremy and the team need to just discuss and make some executive decisions here.

Sy

On Sun, Feb 2, 2020 at 9:58 AM dimacio notifications@github.com wrote:

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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/133?email_source=notifications&email_token=ABAOWUOE35S44USGWDQLAHDRA3NSJA5CNFSM4J4MOPX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKRYXSY#issuecomment-581143499, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUPQCH7ISKCURJPZZDLRA3NSJANCNFSM4J4MOPXQ .

-- Professor Simon Billinge Columbia University

[jlhitt1993]

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]

Yeah support for minimum seems fine.