grid-x / modbus

BSD 3-Clause "New" or "Revised" License
78 stars 26 forks source link

Implement Modbus UDP #85

Closed StefanNienhuis closed 5 months ago

StefanNienhuis commented 6 months ago

This PR implements Modbus RTU over UDP.

Also implements handling of UDP addresses in Modbus CLI and documents this usage in the README.

This solves #50.

StefanNienhuis commented 6 months ago

@tretmar Thanks for the review. I've made the changes.

andig commented 5 months ago

What's missing to get this merged?

andig commented 5 months ago

@StefanNienhuis after looking through the code (and not being a network protocol guy) I have a situation here in https://github.com/evcc-io/evcc/blob/master/meter/goodwe/server.go where we're sending UDP requests to a fixed port and the device replies on a fixed port:

// receiver
addr, err := net.ResolveUDPAddr("udp", "0.0.0.0:8899")
if err != nil {
    return nil, err
}

instance.conn, err = net.ListenUDP("udp", addr)
if err != nil {
    return nil, err
}

// sender
addr, err := net.ResolveUDPAddr("udp", net.JoinHostPort(ip, "8899"))
if err == nil {
        // hand-built RTU
    _, err = m.conn.WriteToUDP([]byte{0xF7, 0x03, 0x89, 0x1C, 0x00, 0x7D, 0x7A, 0xE7}, addr)
}

// listener
buf := make([]byte, 1024)
n, addr, err := m.conn.ReadFromUDP(buf)
if err != nil {
    continue
}

Do you see any chance to support such a scenario? Or maybe I misunderstand this PR or UDP in general?

StefanNienhuis commented 5 months ago

@andig I’m not quite sure what your goal is, but I see it’s intended for Goodwe inverters, which is exactly what I built this for. The only thing that would be missing is supporting the AA55 prefix it sends before responses on some models, but that may not be required for your model.

andig commented 5 months ago

I think I'm confused by the fact that your code does not have a UDP listener, but I guess I'm just over-complicating UDP mentally.

StefanNienhuis commented 5 months ago

Go abstracts away large parts of the UDP protocol details in their network implementation. This allows you to have a UDP ‘connection’ that can send and receive data similar to TCP, even though there isn’t a persistent connection.

dammarco commented 5 months ago

Sorry, it is common for us that the authors themselves are merging the PR but I guess you either are not allowed or just not think about merging your own PRs 🤔

As this has no impact for us, I will merge it now :)

andig commented 5 months ago

@StefanNienhuis confirmed working in https://github.com/evcc-io/evcc/discussions/12752#discussioncomment-9127102. Unfortunately we're now confronted with https://github.com/grid-x/modbus/pull/85#issuecomment-2052042978. Seems forking would be the right way to go to implement a workaround for this device defect.

StefanNienhuis commented 5 months ago

Yeah, although it's pretty trivial to implement, it may be out of scope for this project to implement such device specific behavior. Not sure who decides what goes in this project and what not, but since you merged this PR, what do you think @tretmar?

In case you're not familiar, some families of Goodwe inverters accept their commands in normal Modbus format, but send their response in Modbus format with the bytes AA55 prefixed.

dammarco commented 4 months ago

I'd say that device specific behavior doesn't belong into this project here 🤔

Forking brings the risk of getting out of sync really quickly. Another idea is to create an option that you can configure that can deal with the response prefix of AA55. And only use it for your project then.

andig commented 4 months ago

Sorry, it is common for us that the authors themselves are merging the PR but I guess you either are not allowed or just not think about merging your own PRs 🤔

@tretmar nobody contributing here has that permission. See https://github.com/grid-x/modbus/issues/45#issuecomment-1002671524, three years old. There's still a queue of PRs that need attention like the important https://github.com/grid-x/modbus/pull/81.

Forking brings the risk of getting out of sync really quickly.

Not responding brings the risk of forking.

Another idea is to create an option that you can configure that can deal with the response prefix of AA55. And only use it for your project then.

How would I do that without a fork or a merged PR?

dammarco commented 4 months ago

How would I do that without a fork or a merged PR?

Well, either forking or merged PR ofc. Sure, if you face the issue that you have to wait multiple months to get it merged, I understand your concerns.