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 support for KMTronic usb relay controller #1568

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

benjamin1313
Copy link

@benjamin1313 benjamin1313 commented Jan 16, 2025

This adds support for KMTronic usb relay controller.

Tests has been done using a KMTronic four channel usb relay controller: https://kmtronic.com/product/2786/usb-relay-controller-four-channel-box.html
Small tests for the resource and driver has been written and run with pytest to see in the driver works.
A small setup with coordinator and exporter has also been setup to make sure network resources are available and work with the driver,

Checklist

  • Documentation for the feature

  • Tests for the feature

  • The arguments and description in doc/configuration.rst have been updated
  • PR has been tested

@benjamin1313
Copy link
Author

I will note i needed to install pyserial(outside of venv) on my exporter in order to be able to use the driver and resource over the network.

@benjamin1313 benjamin1313 force-pushed the kmtronic branch 2 times, most recently from 92246ea to 1d32aeb Compare January 17, 2025 08:26
@benjamin1313 benjamin1313 force-pushed the kmtronic branch 2 times, most recently from 7e97856 to 97c1379 Compare January 28, 2025 13:42


def test_kmtronicrelay_resource(target):
r = KMTronicRelay(target, name=None, match={"ID_SERIAL_SHORT": "AB0LBF2U"})
Copy link

Choose a reason for hiding this comment

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

I catch the errors on this test:

$ pytest tests/test_kmtronicrelay.py::test_kmtronicrelay_resource
...
E           TypeError: ("'ports' must be <class 'int'> (got None that is a <class 'NoneType'>).", Attribute(name='ports', default=None, validator=<instance_of validator for type <class 'int'>>, repr=True, eq=True, eq_key=None, order=True, order_key=None, hash=None, init=True, metadata=mappingproxy({}), type=None, converter=None, kw_only=False, inherited=False, on_setattr=None, alias='ports'), <class 'int'>, None)

The reason is

class KMTronicRelay(USBResource):
    ...
    ports = attr.ib(default=None, validator=attr.validators.instance_of(int))

If to be honest, I don't understand this definition. In my sense, default=None and attr.validators.instance_of(int) are incompatible.

Arguably should set default=1 for ports?

Copy link
Author

Choose a reason for hiding this comment

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

Can see i forgot to set ports in the test.
The reason for the default=None is to force the user to select the amount of ports their kmtronic supports since in this way i will know how many bytes to read when using the read all command.

Assuming 8 ports on the kmtronic will result in the driver hanging while waiting for more bytes if a user plugs in a kmtronic with less that 8 ports.

And while assuming 1 does seem fair and I'm not that much against it. It does mean the driver only reads 1 byte from the kmtronic relay and if it only reads one byte at a time we might get problems where it reads the byte value from another relay next time the driver tries reading. A plausible fix for this is clearing the buffer before reading.
That just leaves the fact the user might be a bit confused if the don't set the port. can set the state of a relay but not read it if it is more that 1.
So to keep it simple I made the default none. this creates an error so a user will have to set ports.

Copy link

@cidlik cidlik Feb 4, 2025

Choose a reason for hiding this comment

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

Ok, TY for answer.

The reason for the default=None is to force the user to select the amount of ports their kmtronic supports since in this way i will know how many bytes to read when using the read all command.

It seems to me a little non-obviously, that some argument has default, but I whatever must redefine it. In this approach default using is excess, IMHO.
But I see that there are several resources with the same "feature". Maybe there is extra meaning in this approach, but I don't understand it yet.

@@ -9,13 +9,14 @@ def set_output(self, path, index, status):
with serial.Serial(path, 9600) as ser:
ser.write(cmd)

def get_output(self, path, index):
def get_output(self, path, index, ports):
Copy link

@cidlik cidlik Feb 4, 2025

Choose a reason for hiding this comment

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

It breaks the working with USB One Relay https://forums.raspberrypi.com/viewtopic.php?t=125621
I don't have four channel relay for testing, and link from PR description is broken, thus I don't know the commands.
If commands differ for one and four channels relays, I suggest to add branch:

        if ports == 1:
            # Branch for KMTronic USB One Relay
            cmd = bytes([255, 1, 3])
            with serial.Serial(path, 9600) as ser:
                ser.write(cmd)
                data = ser.read(3)
            return data[2]

Copy link
Author

Choose a reason for hiding this comment

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

Seems like there has been made some changes on the site. Here is a new link https://kmtronic.com/product/2786/usb-relay-controller-four-channel-box.html

Originally i used cmd = bytes([255, index, 3]) This is a single read command and only reads from the specified relay.
This works on the 4 port kmtronic I have primarily been testing on. But when a client i'm making this for tried on a 8 port kmtronic this did not work. After writing to kmtronic we found the 8 port version does not support single reads. We where told a read all command could be used. FF 09 00 can be used to get the state from all relays on the kmtronic,
Returned as xx xx xx xx xx xx xx xx where 01 is on and 00 is off. Sadly it seems there are problems with the links to the docs where this is noted.

Now i don't have a 1 or 2 port kmtronic to test on those. but my hope was the FF 09 00 command to read from all would work on the 1 and 2 port as well. If you have a 1 port are you able to test this?

Copy link

Choose a reason for hiding this comment

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

but my hope was the FF 09 00 command to read from all would work on the 1 and 2 port as well.

My case is inverse, I only have one channel relay. And I couldn't work with it.
I tried to add the conditional branch from the previous comment, and it works! FF 09 00 command is ignored, driver hangs on in reading state (there is no response from the device?)

Copy link
Author

Choose a reason for hiding this comment

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

Okay, there's not much to be done about that. Thanks for letting me know.
Do you know if the two port has support for reading from a single relay and or reading from all?

Copy link

Choose a reason for hiding this comment

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

Unfortunately, no, I don't know. The only thing that could have helped me, it's a page [1]. But it doesn't have any information about reading.
I would offer you to add extra branch and to print warning or to raise an exception like "we don't know how it works with 2-channel relay".

[1] https://kmtronic.com/product/2783/usb-relay-controller-two-channels.html

Copy link
Author

Choose a reason for hiding this comment

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

I wrote an email to KMTronic Technical support. And they tell me only the FF 0x 03 read command is supported on the usb 2 relay controller. Seems going back to FF 0x 03 being the default is the way to go. Then if port is set to something other that 1 it tries to read from that number of ports using FF 09 00

Copy link
Author

Choose a reason for hiding this comment

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

maybe with a warnning on 2 port since it is not supported on that one.

Copy link
Author

Choose a reason for hiding this comment

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

@cidlik would you be able to check if the new changes work on the 1 relay controller?

Copy link

@cidlik cidlik Feb 6, 2025

Choose a reason for hiding this comment

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

I did it. Verified, it works. TY

@@ -110,6 +110,11 @@ def test_all_modules():
methods = aw.list()
assert 'usb_hid_relay.set' in methods
assert 'usb_hid_relay.get' in methods
aw.load('kmtronix_relay')
Copy link

@cidlik cidlik Feb 4, 2025

Choose a reason for hiding this comment

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

Should be kmtronic_relay?

$ pytest tests/test_agent.py::test_all_modules
...
>       with open(filename, 'r') as source_fd:
E       FileNotFoundError: [Errno 2] No such file or directory: '/home/rkuznecov/3dpty/labgrid/.venv/lib/python3.12/site-packages/labgrid/util/agents/kmtronix_relay.py'

@@ -517,6 +517,24 @@ def _get_params(self):
"index": self.local.index,
}

@attr.s(eq=False)
class KMTronicRelayExport(USBGenericExport):
"""ResourceExport for outputs on KMtronic relays"""
Copy link

Choose a reason for hiding this comment

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

Should be KMTronic?

@benjamin1313 benjamin1313 force-pushed the kmtronic branch 2 times, most recently from 3527550 to 697c72e Compare February 4, 2025 13:40
Signed-off-by: Benjamin B. Frost <[email protected]>
kmtronic usb 8 relay controller only support reading from all relays at once.
kmtronic usb 1 and 2 relay controller only support reading from one relay at a time.
And kmtronic usb 4 relay controller supports reading using both methods.

Signed-off-by: Benjamin B. Frost <[email protected]>
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