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

Totalizer Batch Volume functionality #113

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

avichalk
Copy link
Contributor

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

Added functions to get and set the totalizer batch volume on controllers.
@alexrudd2
Copy link
Member

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
Copy link
Contributor Author

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!

  • Avi

@alexrudd2
Copy link
Member

alexrudd2 commented Mar 30, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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!

Added unit_value parameter.
@avichalk
Copy link
Contributor Author

avichalk commented Apr 1, 2024

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
Copy link
Member

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.

@alexrudd2 alexrudd2 merged commit bd69bcb into numat:master Apr 2, 2024
10 checks passed
@avichalk
Copy link
Contributor Author

avichalk commented Apr 2, 2024

Got it, thank you!

@avichalk avichalk deleted the avichalk-patch-1 branch April 2, 2024 19:44
@avichalk avichalk restored the avichalk-patch-1 branch April 2, 2024 19:44
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.

2 participants