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.

Sign up for GitHub

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

Jump to bottom

Flowmeter #220

Merged
merged 21 commits into from
Feb 11, 2025
Merged

Flowmeter #220

merged 21 commits into from
Feb 11, 2025

Conversation

YannickDieter
Copy link
Contributor

No description provided.

@YannickDieter YannickDieter mentioned this pull request May 7, 2024
Closed
@YannickDieter
Copy link
Contributor Author

Can you please comment a bit on the changes? To me it looks like a lot is simplyfied (most of the code is removed), but it still works?

@YannickDieter
Copy link
Contributor Author

YannickDieter commented May 15, 2024 •
edited
Loading

ping @matthias-schuessler

@matthias-schuessler
Copy link
Contributor

I can't really comment on this, because this branch is entirely Antonios and Thomas work. I never worked with this branch and our current module qc environment does not use the changes in it. I can look into this.

@YannickDieter
Copy link
Contributor Author

YannickDieter commented May 15, 2024 •
edited
Loading

Yes, please have a look. In the longterm we want to use the flowmeter for our setup and it would be nice to check if the code changes actually work. Thanks! :)

AntonioT7 and others added 15 commits July 29, 2024 16:51
@AntonioT7 @matthias-schuessler
Improvement on steering the valve in bronkhorst_ELflow
dc81de9
@AntonioT7 @matthias-schuessler
parent df1180b
a7ac683 
author AntonioT7 <antoniotarzikhan@gmail.com> 1669815880 +0100
committer Matthias <matthias@silab83.physik.uni-bonn.de> 1715085603 +0200

new julabo fp50

new julabo fp50

Update julabo_FP50.yaml

new julabo fp50
@matthias-schuessler
Julabo FP50: update
5a4b139 
julabo FP50: update
@matthias-schuessler
WIP: improve bronkhorst flowmeter implementation
a30cfbf
@matthias-schuessler
WIP: add a link to the commads document
ef13821
@matthias-schuessler
WIP: adding commands
0bb3236
@matthias-schuessler
WIP: delete the functions of get/set control mode written by thirono
a42bfcd
@matthias-schuessler
WIP: bring back the functions from thirono for a while
1df7aae
@AntonioT7 @matthias-schuessler
brokhorst valve control and measure N2 flow is ready
14db023
@AntonioT7 @matthias-schuessler
corrected some comments
bdd1a4f
@SilasM2001 @matthias-schuessler
FIX: fix set_setpoint
31d2013
@SilasM2001 @matthias-schuessler
ENH: fix added get_valve_output
c28940f
@SilasM2001 @matthias-schuessler
Added basic example
3e4a833
@SilasM2001 @matthias-schuessler
fixing codestyle
f2a834c
@SilasM2001 @matthias-schuessler
fixing codestyle
254ab29
@cbespin
Copy link
Contributor

cbespin commented Jul 29, 2024

@matthias-schuessler Why do you always force-push? If it does not work with a regular push, your local history is messed up.

@matthias-schuessler
Copy link
Contributor

matthias-schuessler commented Jul 29, 2024 •
edited
Loading

@cbespin I did this, since I just performed a rebase of flowmeter onto the current version of master (basil release 3.3.0). From what I gathered from documentation, a force-push is necessary after a rebase since the idea behind rebase is to change the branch history.

@matthias-schuessler
Copy link
Contributor

Yes, please have a look. In the longterm we want to use the flowmeter for our setup and it would be nice to check if the code changes actually work. Thanks! :)

The flowmeter changes in here are working properly after rebase and can be considered ready for merge.

@cbespin cbespin self-requested a review July 29, 2024 15:07
cbespin
cbespin requested changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@cbespin cbespin left a comment

Choose a reason for hiding this comment

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

Nice PR, nice cleanup and comments. Makes the code much more readable! I have a few questions and suggestions that I am open to discuss.

Also nice to see a new example, thanks!

basil/HL/bronkhorst_elflow.py Show resolved Hide resolved
basil/HL/bronkhorst_elflow.py Show resolved Hide resolved
basil/HL/bronkhorst_elflow.py Outdated Show resolved Hide resolved
basil/HL/bronkhorst_elflow.py Outdated Show resolved Hide resolved
cbespin
cbespin approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@cbespin cbespin left a comment

Choose a reason for hiding this comment

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

All questions answered now

@cbespin cbespin merged commit fa854fe into master Feb 11, 2025
5 checks passed
@cbespin cbespin deleted the flowmeter branch February 11, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SilasM2001 SilasM2001 SilasM2001 left review comments

@matthias-schuessler matthias-schuessler matthias-schuessler left review comments

@cbespin cbespin cbespin approved these changes

Assignees

@matthias-schuessler matthias-schuessler

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@YannickDieter @matthias-schuessler @cbespin @SilasM2001 @AntonioT7