numat / alicat

Python driver and command line tool for Alicat mass flow controllers.
GNU General Public License v2.0
21 stars 27 forks source link

Totalizer Batch Volume functionality #113

Closed avichalk closed 3 months ago

avichalk commented 3 months ago

Added functions to get and set the totalizer batch volume on controllers.

alexrudd2 commented 3 months ago

Hello @avichalk.

Thank you for the pull request.

Can you describe how you made the changes? For some reason git thinks the entire file is changed! Did you use a git-aware editor, or download and re-upload the entire file? I'll have to figure out what's going on here.

Have you tested this on actual hardware?

avichalk commented 3 months ago

Hi Alex,

Yep, downloaded and reuploaded the entire file. This was a quick thing for a customer so I made the changes in notepad.

There's 30 lines of new code in two functions, "get_totbatch" and "set_totbatch". Nothing else has been changed. These changes just allow for reading the totalizer batch volume and setting it using python.

This has been tested on actual hardware and it worked. Let me know if I should change anything or use a git aware editor and I will do so.

Thanks!

alexrudd2 commented 3 months ago

Yep, downloaded and reuploaded the entire file. This was a quick thing for a customer so I made the changes in notepad.

Aha, that's what did it. The line endings in this repo are LF, but I bet Notepad changed them to CRLF. So git sees literally every line as a change. Aside: I think the very newest versions of Notepad (or any version of Notepad++) allow changing line endings in the lower right.

alexrudd2 commented 3 months ago

There's 30 lines of new code in two functions, "get_totbatch" and "set_totbatch". Nothing else has been changed. These changes just allow for reading the totalizer batch volume and setting it using python.

Thank you; after the line ending changes your code is clear. I hope you don't mind me making some formatting and style changes. Note I've changed the function signature! So you may need to adjust your calling code.

This has been tested on actual hardware and it worked. Let me know if I should change anything or use a git aware editor and I will do so.

Can you double-check that f'{self.unit} TB {batch}' is correct? The serial primer doesn't show a space between the unit ID and TB. image

Is it worth adding the unit_value parameter?

alexrudd2 commented 3 months ago

This was a quick thing for a customer

do you mind sharing the general idea of that relationship? I'm mostly interested in who is using the code, and what their needs are :)

avichalk commented 3 months ago

Hi Alex,

Ah, got it. I'll check out Notepad++, thanks! The space didn't make a difference but it shouldn't hurt to remove it to be consistent with the rest of the code. I'll have to do some more testing to add unit_value, let me get back to you about that on Monday. Thanks for all the help! And reformatting is no problem. I'm always looking to learn best practices :)

The customer uses the Python package for all comms with their devices, and emailed in asking us if we supported the totalizer batch volume functionality, since they use it a lot in their process. I realized that it wouldn't be too bad to implement, so I went ahead and added it in. They were fine with the volume being the default units on the device which is why unit_value was not yet implemented.

Let me know if there's any specific information I should get. And thanks again!

avichalk commented 3 months ago

I've gone ahead and added the unit_value functionality discussed above, and made sure the line endings are LF this time. This was tested on hardware and worked. Let me know if there's anything else I should do. Thanks again!

alexrudd2 commented 3 months ago

Everything looks good to me, so I'll get this released on PyPI. Thanks for your contribution!

If you're going to continue working with Python for this customer, I'd recommend getting a powerful editor or IDE like Pulsar, VSCodium, or even PyCharm. They have lots of features like git integration and automatically checking style changes that make development faster.

avichalk commented 3 months ago

Got it, thank you!