interface thoughts #62

New Issue

hartytp · 2022-01-05T03:43:59+08:00

hartytp commented

2022-01-05 03:43:59 +08:00

This is a bit of a brain dump of having spent some time playing with Thermostat today, but I was a bit dissapointed in the interface...

It exposes implementation details to the user which should be abstracted over. For example, IMHO the user should not see that the current limit pins are driven by PWM. It's unneccecary detail which one doesn't need to know to understand the device's function and just adds confusion.

The documentation around the functions is unclear. What does Set PWM duty cycle for max_i_pos to ampere mean? What does setting a duty cycle to ampere mean? The user needs to understand quite a lot about the device implementation to guess what is going on here. It would be much better to document what this actually does rather than how it does it. Also, to double check, these functions only set the voltages to the MAX chip, right? i.e. they are just hardware limits and don't affect integrator wind-up limits, or the value of i_set the software will produce, right? i.e. even if one sets the current limit to 0.1A, i_set can still reach 3A, right?

It's confusing that there are so many different output settings (max_i_pos, output_min, integral_min). Is there ever a good reason to have these separate? If not, shouldn't the interface set them all at once?

Is there documentation for the minimal set of settings to make a device work?

The meaning of i_set seems to be different for setting (where it's a current AFAICT) and readback (where it's a DAC voltage AFAICT). This is rather confusing.

IMHO report mode is an anti-feature. It's pretty much always preferable to use the "bare" mode where one asks for a temperature and gets a single answer.

The pytec Client does not provide (that I could see) a method for querying the temperature other than using report mode. It also does not provide any means of disabling report mode. Once report mode is on, other functions start misbehaving (e.g. get_pwm does not always return something). Report mode also seems to yield each temperature measurement twice.

This is a bit of a brain dump of having spent some time playing with Thermostat today, but I was a bit dissapointed in the interface... It exposes implementation details to the user which should be abstracted over. For example, IMHO the user should not see that the current limit pins are driven by PWM. It's unneccecary detail which one doesn't need to know to understand the device's function and just adds confusion. The documentation around the functions is unclear. What does `Set PWM duty cycle for max_i_pos to ampere` mean? What does setting a duty cycle to ampere mean? The user needs to understand quite a lot about the device implementation to guess what is going on here. It would be much better to document what this actually does rather than how it does it. Also, to double check, these functions *only* set the voltages to the MAX chip, right? i.e. they are just hardware limits and don't affect integrator wind-up limits, or the value of i_set the software will produce, right? i.e. even if one sets the current limit to 0.1A, i_set can still reach 3A, right? It's confusing that there are so many different output settings (max_i_pos, output_min, integral_min). Is there ever a good reason to have these separate? If not, shouldn't the interface set them all at once? Is there documentation for the minimal set of settings to make a device work? The meaning of i_set seems to be different for setting (where it's a current AFAICT) and readback (where it's a DAC voltage AFAICT). This is rather confusing. IMHO report mode is an anti-feature. It's pretty much always preferable to use the "bare" mode where one asks for a temperature and gets a single answer. The pytec Client does not provide (that I could see) a method for querying the temperature other than using report mode. It also does not provide any means of disabling report mode. Once report mode is on, other functions start misbehaving (e.g. get_pwm does not always return something). Report mode also seems to yield each temperature measurement twice.

hartytp commented

2022-01-06 01:57:11 +08:00

Conclusion for anyone looking over these issues: to my view this is not in state where I would expect a skilled scientist (e.g. has used commercial temperature controllers before) to be able to easily get this working. If I wasn't very familiar with the hardware design and willing to dig into the firmware I don't think I would have got any life out of it before giving up.

sb10q commented

2022-01-06 09:31:47 +08:00

I disagree with that conclusion, there are several users who have it in the lab without complaining like you do - they might just have to send us a question or two (that's what we have helpdesk@ for).
How about you send PRs or propose concrete/detailed changes instead of bashing the project? As you can see with WRPLL, developing things are harder than you think.

I disagree with that conclusion, there are several users who have it in the lab without complaining like you do - they might just have to send us a question or two (that's what we have helpdesk@ for). How about you send PRs or propose concrete/detailed changes instead of bashing the project? As you can see with WRPLL, developing things are harder than you think.

sb10q commented

2022-01-06 09:59:58 +08:00

I'll have someone look into the helpful technical content of your messages, but just like you we have turnover issues (and unlike you we are on the wrong end of HK emigration) and it may take a while.

hartytp commented

2022-01-06 15:38:35 +08:00

I disagree with that conclusion, there are several users who have it in the lab without complaining like you do - they might just have to send us a question or two (that's what we have helpdesk@ for). How about you send PRs or propose concrete/detailed changes instead of bashing the project?

hmmmm....

This isn't "bashing the project" (in case it's not clear, I have invested a lot of my time and energy into this project and am personally motivated to see it work). I'm happy to amend my previous statement to add "without getting support." but the point stands: I had hoped that a skilled experimentalist with limited knowledge of the specifics of this hardware design (or rust skills) would be able to get a Thermostat up and running and temperature stabilize things in a couple of hours. That is not the case currently. If we'd known that up front we would have project managed things differently; it would have been useful information.

If people are using this then great, but presumably they hit similar issues to me? If they're resolving those issues via email without leaving any public record that's a real shame IMHO. If there had been an open issue saying "the python example code is broken right now, just send the commands directly to the device yourself" it would have saved me a few hours of debugging yesterday.

> I disagree with that conclusion, there are several users who have it in the lab without complaining like you do - they might just have to send us a question or two (that's what we have helpdesk@ for). How about you send PRs or propose concrete/detailed changes instead of bashing the project? hmmmm.... This isn't "bashing the project" (in case it's not clear, I have invested a lot of my time and energy into this project and am personally motivated to see it work). I'm happy to amend my previous statement to add "without getting support." but the point stands: I had hoped that a skilled experimentalist with limited knowledge of the specifics of this hardware design (or rust skills) would be able to get a Thermostat up and running and temperature stabilize things in a couple of hours. That is not the case currently. If we'd known that up front we would have project managed things differently; it would have been useful information. If people are using this then great, but presumably they hit similar issues to me? If they're resolving those issues via email without leaving any public record that's a real shame IMHO. If there had been an open issue saying "the python example code is broken right now, just send the commands directly to the device yourself" it would have saved me a few hours of debugging yesterday.

hartytp commented

2022-01-06 15:39:56 +08:00

(I probably will file a PR at some point but first I need to turn my debugging hacks into something useful. Even without a PR right now it's useufl to have the issues open so there is a record of where the rough edges are.)

hartytp commented

2022-01-06 15:41:30 +08:00

it may take a while.

No hurry from my end. We now know enough to get things working so this isn't urgent. The interface exposed by Thermostat is currently overly complex and a bit ugly but we can wrap that in a simple python class and forget about it.

> it may take a while. No hurry from my end. We now know enough to get things working so this isn't urgent. The interface exposed by Thermostat is currently overly complex and a bit ugly but we can wrap that in a simple python class and forget about it.

topquark12 was assigned by sb10q

2022-01-19 11:32:15 +08:00

topquark12 commented

2022-06-07 14:21:26 +08:00

3f6419835f contains pytec/tecQT.py , a GUI for easy configuration, auto tuning (PID) and plotting of thermostat parameters.

The script uses pyqtgraph, which will be automatically installed and configured using nix develop.

Despite internal testing did not reveal major issues with the GUI, the GUI is still considered experimental and care should be taken when testing it. Avoid testing the GUI with delicate loads just in case.

Testing and feedback regarding the GUI is greatly appreciated.

3f6419835f contains pytec/tecQT.py , a GUI for easy configuration, auto tuning (PID) and plotting of thermostat parameters. The script uses pyqtgraph, which will be automatically installed and configured using nix develop. Despite internal testing did not reveal major issues with the GUI, the GUI is still considered experimental and care should be taken when testing it. Avoid testing the GUI with delicate loads just in case. Testing and feedback regarding the GUI is greatly appreciated.

Screenshot from 2022-06-07 14-08-51.png

330 KiB