Skip to content

Conversation

@zoltuz
Copy link
Contributor

@zoltuz zoltuz commented Feb 4, 2020

Hi Harrison,
First of all, DO NOT MERGE THIS PR with the master for now. I just wanted to show you a path I'd like to take with the code, namely allowing for config files for certain variables (two vs four pump, number of OD measurements, etc).
I've not tested this code below. I'll do it later this week, once I get access to the chi.bios, until then I'm happy to get some feedback from you.

Cheers,
Zoltan

- creating a placeholder for DeviceName for later use
- check config file for config key, raise ValueError if critical config data is missing
- number of OD measurements set by the config file
@zoltuz zoltuz requested a review from HarrisonSteel February 4, 2020 15:12
@HarrisonSteel
Copy link
Owner

Hi Zoltan,

I think it looks like a sensible and useful feature to add, certainly will be helpful if people are frequently setting up the same experiment etc. Let me know how the testing goes!

- moving the config value to the initialization phase
@zoltuz
Copy link
Contributor Author

zoltuz commented Feb 6, 2020

I've tested the code as much as I can. It handles well the case when a config value was not found. Here're the two cases:
[2020-02-06 16:21:18,857] WARNING in app: config value TWO_PUMPS_PER_DEVICE was not found and set to default: 0
[2020-02-06 16:21:18,860] INFO in app: config found: NUMBER_OF_OD_MEASUREMENTS=4
The code still has the default values and it defaults back to it when the whole config file is missing or any given settings (that is actually used in the code).

Let me know you think!

Zoltan

@zoltuz zoltuz marked this pull request as ready for review February 6, 2020 18:00
@zoltuz
Copy link
Contributor Author

zoltuz commented Feb 15, 2020

Hi Harry,
We've run this code couple of times this code without any software errors. Unfortunately, there's no unit tests, so I can't say I've tested all aspects of the code. If you want you can also grab this code and run it on your ChiBios just to see everything is ok.

if application.config['TWO_PUMPS_PER_DEVICE']:
chibios_to_shut_down = [0, 1, 2, 3]
else:
chibios_to_shut_down = [0, 1, 2, 4, 5, 6, 7]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

number 3 is missing!

chibios_to_shut_down = [0, 1, 2, 3]
else:
chibios_to_shut_down = [0, 1, 2, 4, 5, 6, 7]
if int(M[1]) in chibios_to_shut_down:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's use f-string instead! printf(f'M{number}') in number

setPWM(M,'Pumps',sysItems['All'],0,0)

if application.config['TWO_PUMPS_PER_DEVICE']:
chibios_to_shut_down = [0, 1, 2, 3]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's call the variable chibios_w_pumps

sysData[M]['present']=0
tries=-1
if tries>10: #In this case something else has gone wrong, so we panic.
if tries >= application.config['DEVICE_COMM_FAILURE_THRESHOLD']: #In this case something else has gone wrong, so we panic.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove this!

if(sysData[M]['present']==1):
turnEverythingOff(M)
print(str(datetime.now()) + " Initialised " + str(M) +', Device ID: ' + sysData[M]['DeviceID'])
print(str(datetime.now()) + " Initialised " + str(M) + ', (' + sysData[M]['DeviceName'] + ') Device ID: '
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove device name feature

@zoltuz
Copy link
Contributor Author

zoltuz commented Jun 25, 2021

add CONTINUOUS_STIRRING = False to this PR

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