Integration of Power Meter HP437B

[neuschs]

No description provided.

[LongnoseRob]

Thank you for the contribution,

i left some comments in addition to the points found by the CI.

On the actual device, have you had a look inot it how much difference there is to the HP43 (with HPIB/GPIB option) and or the dual-channel 438?

[neuschs]

Thank you for the contribution,

i left some comments in addition to the points found by the CI.

On the actual device, have you had a look inot it how much difference there is to the HP43 (with HPIB/GPIB option) and or the dual-channel 438?

I had a short look over the HP438A because it got mentioned in the manual of the 437B.
And indeed they do share a lot of common commands, but I so far have no good idea on how to combine these two.
The HP438A is two-channel where you have to send a message in before every other command to determine which channel you want to configure or measure.
But on the 437B this issues doesn't exist.
And additionally implementing a device without being able to test it even once feels wrong.

What do you mean with "HP43 (with HPIB/GPIB)" ? The 437B as well as the 438A have GPIB options.

[LongnoseRob]

Thank you for the contribution,
i left some comments in addition to the points found by the CI.
On the actual device, have you had a look inot it how much difference there is to the HP43 (with HPIB/GPIB option) and or the dual-channel 438?

I had a short look over the HP438A because it got mentioned in the manual of the 437B. And indeed they do share a lot of common commands, but I so far have no good idea on how to combine these two. The HP438A is two-channel where you have to send a message in before every other command to determine which channel you want to configure or measure. But on the 437B this issues doesn't exist. And additionally implementing a device without being able to test it even once feels wrong.

Yes, without access to the hardware for testing it makes little sense.

What do you mean with "HP43 (with HPIB/GPIB)" ? The 437B as well as the 438A have GPIB options.

Sorry, I ment the HP436..

[neuschs]

Yes, without access to the hardware for testing it makes little sense.

I can pack the common commands into a base class so someone can later reuse them for the HP438A. But that would only make sense if it makes sense to inherit from the Channel and the BaseClass for the 438A because that one has the two channels.

I checked, the HP436A shares nothing in common with the HP437B.

[LongnoseRob]

Yes, without access to the hardware for testing it makes little sense.

I can pack the common commands into a base class so someone can later reuse them for the HP438A. But that would only make sense if it makes sense to inherit from the Channel and the BaseClass for the 438A because that one has the two channels.

I checked, the HP436A shares nothing in common with the HP437B.

It was just an idea but if it is too much work for the 438, then continue with your initial approach,
If someone then wants to interface the 438, they can then think abiut the channel stuff at that time.

[neuschs]

@LongnoseRob

I created a mockup up of how a 438A combination could be implemented. What do you think ? The multi inherence is kind of awkward.

from pymeasure.instruments import Instrument, Channel
from pymeasure.adapters import ProtocolAdapter
 

class HP43XXGeneralStuff:

    @property
    def test_general(self):
        return "value"

 
    @test_general.setter
    def test_general(self, value):
        self.write(value)


class HP43XXChannelStuff:

    @property
    def test_property(self):
        return "value"

 
    @test_property.setter
    def test_property(self, value):
        self.write(value)
 

class HP438AChannel(Channel, HP43XXChannelStuff):


    @HP43XXChannelStuff.test_property.setter
    def test_property(self, value):
        self.write(f"SET{self.id}")
        HP43XXChannelStuff.test_property.fset(self, value)


class HP438A(Instrument, HP43XXGeneralStuff):
    a = Instrument.ChannelCreator(HP438AChannel, "A")
    b = Instrument.ChannelCreator(HP438AChannel, "B")
 

class HP437B(Instrument, HP43XXGeneralStuff, HP43XXChannelStuff):
    pass


hp438a = HP438A(ProtocolAdapter(), name="Test")
hp438a.a.test_property = "Value"
hp438a.test_general = "other value"
 
hp437b = HP437B(ProtocolAdapter(), name="Test")
hp437b.test_property = "Value"
hp437b.test_general = "other value"

[BenediktBurger]

A few, quick comments regarding inheritance without looking at the code in detail:
In your mockup, the xx channel / instrument could already inherit from Channel / Instrument, which would make multiple inheritance superfluous.
Would it be feasible to make the 437 (which has those additional commands) inherit from the 438?

A comment regarding testing without a device: for that reason we have protocol tests.
I do not know whether the instrument has tests. If not, you could write tests, before you touch the instrument. After changing it, the tests have to pass again. That ensures, that the communication did not change during code refactoring.

[neuschs]

In your mockup, the xx channel / instrument could already inherit from Channel / Instrument, which would make multiple inheritance superfluous.

Sadly no, because the 437B is no multiple channel device. It has only one power sensor connector and therefore shouldn't offer the channel namespace.
But the 438A has two channels.

Would it be feasible to make the 437 (which has those additional commands) inherit from the 438?

The 438 has additional commands and has two channs.

I do not know whether the instrument has tests. If not, you could write tests, before you touch the instrument. After changing it, the tests have to pass again. That ensures, that the communication did not change during code refactoring.

I agree !

[LongnoseRob]

Thank you for the contribution, in gerneral it looks good to me.
I left some comments on certain points where iimprovement could be discussed.

[LongnoseRob]

Thanks for the latest changes, LGTM

[BenediktBurger]

Thanks for your contribution, I commented on a few points to improve.

[neuschs]

@BenediktBurger Can you have a look why that Python 3.8 macos-latest test fails ?

[BenediktBurger]

@BenediktBurger Can you have a look why that Python 3.8 macos-latest test fails ?

The test succeeded.

[BenediktBurger]

Two more small changes, please.

[BenediktBurger]

May I merge?

[neuschs]

Sure go on