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

Add service for AimTTiPL-P #149

Merged
merged 53 commits into from
Apr 2, 2024
Merged

Add service for AimTTiPL-P #149

merged 53 commits into from
Apr 2, 2024

Conversation

ivalaginja
Copy link
Collaborator

@ivalaginja ivalaginja commented Dec 10, 2023

Fixes #148.

Manufacturer info:
https://www.aimtti.com/product-category/dc-power-supplies/aim-plseries

Note how "The New PL-P Series is the programmable (remote control) version of the New PL Series [...]", since the above website lists both variants (PL and PL-P).

Usage example for the dcps library taken from here:
https://github.com/sgoadhouse/dcps/blob/master/dcps/AimTTiPLP.py

Edit: Could also be interesting to look into https://msl-equipment.readthedocs.io/en/latest/_api/msl.equipment.resources.aim_tti.html#module-msl.equipment.resources.aim_tti

Todo:

  • Generalize to be able to control all channels of one device
  • Add methods to measure current and voltage
  • Check current command units
  • Test on hardware

Notes:

  • The device automatically applies a remote interface lock when it is commanded for the first time (see manual, page 23). This is also noted in the dcps library. Even after setting the interface lock to LOCAL, this will instantly be overwritten back to REMOTE when a new command is issued.
  • All remote commands are listed on page 34 of the device manual.
  • I only implemented a minimum of the commands listed in the manual. More commands can easily be added in the future as needed.

@ivalaginja ivalaginja self-assigned this Dec 10, 2023
@ivalaginja ivalaginja added hardware Integrate new hardware collaborators Worked on by external collaborator. Might need some extra help on code integration. labels Dec 10, 2023
from dcps import AimTTiPPL


class AimTtiPlp(Service):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I like this class name...

Copy link
Collaborator Author

@ivalaginja ivalaginja Mar 10, 2024

Choose a reason for hiding this comment

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

@ehpor should I use this abomination of a class name or do you have other suggestions? :P An alternative would be AimTTiPLP like in the dcps library, although I think that might override it in the name space (same name)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Name doesn't matter to me. I would not use the same name as the library one.

try:
# Get an update for this channel
voltage = self.voltage.get_next_frame(10).data[0]
current = self.current.get_next_frame(10).data[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't work as expected. If you don't get an update frame on the voltage and the current in the same iteration of this loop, then the setVoltage() and setCurrent() will not be called. You should use threads to monitor both. Then, you'd also wanna use a lock to avoid simultaneous access to the physical device from those two threads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, like any other service that needs to monitor several data streams. I also need to extend this to several output channels with two data streams each, but I stopped so I can think about how to do that.
Sounds like it's as simple as having twice as many (monitored) data streams/threads as output channels (each requiring a voltage and current data stream). Is that right? Is there any better way to do that than to call them 'voltage_{channelnumber}' and 'current_{channelnumber}' and spin off a monitoring thread for each?

@ivalaginja
Copy link
Collaborator Author

@ehpor this should now be functional (albeit untested) code, do you agree? I decided to split monitoring methods in two separate ones to save myself the if...elif... to differentiate between the voltage and current data streams, but I don't know if there is any reason to not do it like this. I'd love to hear your opinion.

@ivalaginja
Copy link
Collaborator Author

I might have gone a little too fast with this implementation - it is currently very limited. The power supply in question has various modes it can be run in and I don't think I will be able to implement all possibilities (they're not endless, but about 6 too many compared to what we need).

Being able to switch between constant voltage and constant current mode seems to be the absolute basis, but even so I feel like I would have to add respective data streams and mode switches. Not sure if I should move this over to thd-controls instead and just implement the one mode we're using right now.

I will put this on hold to think about it.

@ivalaginja ivalaginja force-pushed the feature/aim_tti_plp_service branch 2 times, most recently from f34b639 to 1068198 Compare March 14, 2024 12:47
@ivalaginja ivalaginja requested a review from ehpor March 14, 2024 15:07
@ivalaginja
Copy link
Collaborator Author

ivalaginja commented Mar 14, 2024

Hey @ehpor could you help me out with the couple of weird problems I have? I am testing this on hardware right now and I feel like I am very close to make things work.

I manage to establish a connection to the device with this service and get returns from the commands (that do not use data streams). I am equally able to access the data streams of the service, submit to and read from them, however, my device shows no reaction. Since I am perfectly able to control it just by using the code from within the service in a separate terminal through a connection without using catkit2, it looks to me like I messed up the threads and how they're set up with the main function. Do you maybe see what is wrong?

Also, when I Ctlr+C on the server, I can see that this service gets closed down, but it immediately tries to restart which yields a bunch of:

RuntimeError: The testbed is shutting down. Starting a new services is not allowed anymore.

Eventually, the restarting request seems to time out and everything closes all right, but I do not know what is causing this. This also causes the GUI to get stuck, which I have to force shut down.

try:
# Get an update for this channel
frame = self.current_commands[channel_name].get_next_frame(10)
value = frame.data
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: value is a Numpy array here. You probably want frame.data[0].

self.device._instWrite(str)

# Give some time for power suppluy to respond
time.sleep(self.device._wait)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer self.sleep() since this also quits sleeping if the service is being asked to shut down (ie. it checks the shutdown flag internally).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for other places in this file.

@ehpor
Copy link
Collaborator

ehpor commented Mar 14, 2024

Hey @ehpor could you help me out with the couple of weird problems I have? I am testing this on hardware right now and I feel like I am very close to make things work.

I manage to establish a connection to the device with this service and get returns from the commands (that do not use data streams). I am equally able to access the data streams of the service, submit to and read from them, however, my device shows no reaction.

Reading and writing from/to datastreams is completely independent from the service being responsive. No service code is ran when interacting with its datastreams.

Since I am perfectly able to control it just by using the code from within the service in a separate terminal through a connection without using catkit2, it looks to me like I messed up the threads and how they're set up with the main function. Do you maybe see what is wrong?

I looked briefly at the code, but the threads look fine by eye. I found one error though with how you're reading from the datastreams. And one related to sleeping, which is very likely unrelated.

Also, when I Ctlr+C on the server, I can see that this service gets closed down, but it immediately tries to restart which yields a bunch of:

RuntimeError: The testbed is shutting down. Starting a new services is not allowed anymore.

This is likely the GUI trying to restart the service.

Eventually, the restarting request seems to time out and everything closes all right, but I do not know what is causing this. This also causes the GUI to get stuck, which I have to force shut down.

for channel_name in self.channels.keys():
self.set_voltage(channel_name, value=0)
self.set_current(channel_name, value=0)
self.device.outputOff(self.channels[channel_name]['channel'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you're joining the threads.

value = frame.data

# Update the device
with self.lock_for_current:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd move this lock to inside self.set_current() and the same for self.set_voltage(). That offers more protection.

@ivalaginja ivalaginja force-pushed the feature/aim_tti_plp_service branch from b7c38bb to bc06d45 Compare March 15, 2024 16:47
@ivalaginja
Copy link
Collaborator Author

Thanks @ehpor! I will be back in the lab on Monday to test on hardware.

@ivalaginja ivalaginja requested a review from ehpor March 18, 2024 14:24
@ivalaginja ivalaginja marked this pull request as ready for review March 18, 2024 14:24
@ivalaginja
Copy link
Collaborator Author

After some final small fixes, this is now working as expected on hardware.

@ivalaginja
Copy link
Collaborator Author

@ehpor thanks for helping me figure this one out! I had quite some troubles considering what a simple device it is...

@ivalaginja ivalaginja changed the title Add service for AimTTiPLP Add service for AimTTiPL-P Mar 18, 2024
@ivalaginja ivalaginja requested a review from raphaelpclt April 1, 2024 20:58
str = 'TRIPRST'
self.device._instWrite(str)

# Give some time for power suppluy to respond
Copy link
Collaborator

@raphaelpclt raphaelpclt Apr 2, 2024

Choose a reason for hiding this comment

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

Tiny typo in "suppluy"

Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

One general remark, I got a bit confused by all the "channels" names. There's a self.channels dict, that has channel_names dict as keys, that have channel, that is a number. Maybe call everywhere this last-stage channel channel_number everywhere, as it is done in some places? (eg L164)

Apart from that, everything looks good, very tiny typos in commented code.

Comment on lines 171 to 179
"""Attempt to clear all trip conditions.

This only works if the device has not been set up so that it can only reset trip conditions manually on the
front panel, or by cycling the AC power.
"""
str = 'TRIPRST'
self.device._instWrite(str)

# Give some time for power suppluy to respond
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comment: would there be a way to log if it worked or not? Like a feedback from the device saying "Reset trip conditions not possible"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From scanning the manual real quick, it doesn't seem there is. There are ways to get potential errors back however I never tested that.

str = 'OCP{} {}'.format(channel_number, value)
self.device._instWrite(str)

# Give some time for power suppluy to respond
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also tiny typo

str = 'OVP{} {}'.format(channel_number, value)
self.device._instWrite(str)

# Give some time for power suppluy to respond
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy-pasted tiny typo? :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed ^^

@ivalaginja
Copy link
Collaborator Author

One general remark, I got a bit confused by all the "channels" names. There's a self.channels dict, that has channel_names dict as keys, that have channel, that is a number. Maybe call everywhere this last-stage channel channel_number everywhere, as it is done in some places? (eg L164)

Yeah I get that, however the structure with the dict is necessary because I want to have named channels like in the case of the DMs. There, the self.channels dict uses the channel_names as keys that maps to the respective channel number, which I implemented here as well - except I called the numbers just "channels" in the config. Ok yup I see what you mean, I like your suggestion, give me a sec to change things appropriately.

@ivalaginja ivalaginja requested a review from raphaelpclt April 2, 2024 19:08
Copy link
Collaborator

@raphaelpclt raphaelpclt left a comment

Choose a reason for hiding this comment

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

Looks good

@ivalaginja ivalaginja merged commit e320094 into develop Apr 2, 2024
6 checks passed
@ivalaginja ivalaginja deleted the feature/aim_tti_plp_service branch April 2, 2024 19:21
@ivalaginja
Copy link
Collaborator Author

Thanks @raphaelpclt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collaborators Worked on by external collaborator. Might need some extra help on code integration. hardware Integrate new hardware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add service for AimTTiPLP DC power supply
3 participants