Allow sharing transports between packagers by adding Clone method (BC)

[andig]

Fix #56

This is an alternative approach. It features much smaller api surface but leaves responsibility up to the user to understand the transporter's shared nature.

Note: decide name. Is Clone() sufficiently clear? Maybe Share() or ShareConnection() or maybe WithPackager?

[andig]

Friendly ping @frzifus. This would be really helpful using the module downstream and cross-goroutines. The diff looks bigger than it is as I've moved the ...Client routines above the respective ...Handlers for clarity.

[andig]

Any chance for feedback on this change?

[andig]

Friendly ping. This PR is needed to make a modbus connection usable from multiple goroutines, one per id.

[andig]

ping @frzifus did you get a chance to review?

[andig]

ping @frzifus any chance to take a look? Imho this really is a smallish change, although the diff looks large.

[frzifus]

Since users may initialized handlers without the new... functions, I would consider it to mark it as breaking change. wdyt?

The repository does not have a changelog or similar. Maybe we should introduce something like this as an CHANGELOG.md file?

But I assume thats up to @tretmar @Carelo @guelfey - Or maybe you can ping the right contact person.

[frzifus]

Since tcpTransporter doesnt only contain native values, do we need to add a Clone() method for that too?

[andig]

I'm not sure I understand the question (native values?). The purpose of this PR is to share the transporter. The private asciiTCPTransporter etc transporter fields have been converter to pointers to allow this. I didn't see a case for cloning a transporter.

[andig]

@frzifus does that answer the question? Or is there anything specific to the tcpTransporter that I'm missing?

[andig]

In other words: the tcpTransporter is a pointer now that gets copied.

[andig]

TCP is actually a special case that required adjustments to the packager too, see evcc-io#6

[andig]

Since users may initialized handlers without the new... functions, I would consider it to mark it as breaking change. wdyt?

That's a good point. Not doing so will be obvious due to nil pointer panics, but it's definitely breaking. Since this is the case, I'd be happy to add a more fluent way of passing parameters, too if that would help to move this PR along.

[andig]

If anybody wants to test this, I've created a fork at https://github.com/evcc-io/modbus. Happy to update this PR if interest arises.

[andig]

While is PR is meanwhile severely bit-rotten and for sake of reference, we've been successfully using this with evcc.io for ~3 months now and are happy to report that we can have concurrent Go routines interacting with Modbus devices now using different slave ids.

[hnicolaysen]

First of all, thanks for bringing this up @andig!

This PR was long-forgotten on our side, so thank you for your patience and for reviving this! We would be happy to get this repository up to date and to follow up on this change.

Technically, it looks sound to me. I agree, we should mark it as a breaking change unless we come up with a solution that preserves the previous construction.

For me, the name Clone() is fine as the description tells the user well what is does.

[andig]

Thank you @hnicolaysen for the kind words. Lets please try to get #93 merged, then I can update this PR with the remaining fixes. As @frzifus pointed out- though as minimally invasive as possible- this is still a breaking change. Users of the naked structs will now need to initialise the transporters. Due to this it might make sense to merge this only if you're actually going for a v2 version of the library?

[hnicolaysen]

Hi @andig,

since #93 is now closed, what does it mean for this PR? I'm happy to merge it once the PR match the other underlying changes you made.

I would also make a new release as version 2 to make the breaking change more visible 👍

[andig]

Updated PR. Would be great to see the test result.

[dammarco]

Hey @andig, I'll take over from Hendrik now.

I guess you can't look into the pipeline results due to missing permission.
Here is the test results:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
--
  | panic: runtime error: invalid memory address or nil pointer dereference
  | [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x68b99b]
  |  
  | goroutine 7 [running]:
  | testing.tRunner.func1.2({0x6c6640, 0x94f8a0})
  | /usr/local/go/src/testing/testing.go:1545 +0x238
  | testing.tRunner.func1()
  | /usr/local/go/src/testing/testing.go:1548 +0x397
  | panic({0x6c6640?, 0x94f8a0?})
  | /usr/local/go/src/runtime/panic.go:914 +0x21f
  | github.com/grid-x/modbus.(*tcpPackager).Encode(...)
  | /go/src/github.com/grid-x/modbus/tcpclient.go:90
  | github.com/grid-x/modbus.TestTCPEncoding(0xc000309860)
  | /go/src/github.com/grid-x/modbus/tcpclient_test.go:21 +0x9b
  | testing.tRunner(0xc000309860, 0x72f0e8)
  | /usr/local/go/src/testing/testing.go:1595 +0xff
  | created by testing.(*T).Run in goroutine 1
  | /usr/local/go/src/testing/testing.go:1648 +0x3ad
  | exit status 2
  | FAIL	github.com/grid-x/modbus	0.012s
  | make: *** [Makefile:13: test] Error 1
  | make: *** [Makefile:30: ci_test] Error 2
  | 🚨 Error: The command exited with status 2
  | user command error: exit status 2

Hope this helps you and we can move forward with this PR 👍🏻

[pieterhollander]

Hello and thanks for writing this great library.
I'm trying to use it for a project and connection sharing is a requirement for my use case. Is there any reason keeping this commit from being merged and releasing a v2, as it seems that @andig adjusted the tcpclient_test based on the new approach?

It seems that https://github.com/evcc-io/modbus is now both ahead as well as behind this repository in terms of commit and the evcc-io fork still refers to itself as grid-x. Therefore, I'd very much like to use the original grid-x repo if possible.

[andig]

Seems we have diverged due to this PR and #97.

[antoineco]

@pieterhollander I don't think anything is holding us from merging this besides the failing tests. The module has remained unmaintained for a while due to previous maintainers shifting priorities, but IMO what this PR is proposing feels sound.

It seems to me that we could simplify a few things in this module (constructors, access to nested fields) by making struct fields public instead of stacking new methods like this Clone(). I would agree to do refactorings in a subsequent PR though.