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

More than 8 consecutive coil addresses results in the coils with index above 8 to not be detected/polled #37

Closed
Ruaktom opened this issue May 10, 2020 · 1 comment · Fixed by #38
Assignees
Labels
bug Something isn't working

Comments

@Ruaktom
Copy link

Ruaktom commented May 10, 2020

When I have more than 8 consecutive coil addresses in the confi.yml file, this results in groups being created with more than 8 coils.
The coils with index number above 8 are no longer triggered.

Issue can be solved by introducing an additional condition in the GroupCoils function (coilgroup.go) that creates a new group if size of group would exceed 8.

if coil.Address == groups[groupIndex].offset+uint16(len(groups[groupIndex].coils)) && uint16(len(groups[groupIndex].coils)) < 8 {
	// Add the current input to the current group
	groups[groupIndex].coils = append(groups[groupIndex].coils, coil)
} 
else {
	// Start a new group
	groups = append(groups, CoilGroup{offset: coil.Address, coils: []Coil{coil}})
}

Maybe something to be implemented?

@mhemeryck
Copy link
Owner

Hi @Ruaktom! Thx for reaching out. I haven't been looking into this repo for quite some time, so apologies for the late reply.

I was just reading into my own source again. Normally, the length of the coilGroups shouldn't be an issue, since they should be mapped back properly to the right individual coil, see the Update function:

Upon closer inspection however I did notice that the modbus client I am using is returning the data as byte whereas I seemed to have assumed it was uint16:

https://github.com/goburrow/modbus/blob/606c02f4eef527a1d4cf7d8733d5fd7ba34f91d8/client.go#L41

Hence, the mapping back I did in the Update function probably does not match:

This should probably fix it:

diff --git a/coilgroup.go b/coilgroup.go
index 67b65c4..2b2d5d3 100644
--- a/coilgroup.go
+++ b/coilgroup.go
@@ -27,8 +27,8 @@ func (coilGroup *CoilGroup) Update() (err error) {
 		return
 	}
 	for k := range coilGroup.coils {
-		numberIndex := k / 16
-		bitOffset := uint16(k % 16)
+		numberIndex := k / 8
+		bitOffset := uint16(k % 8)
 		value := results[numberIndex] & (1 << bitOffset)
 		coilGroup.coils[k].Update(value != 0, coilGroup.MQTTClient)
 	}

I'll see if I can create test to reproduce the issue plus look into a patch release.

@mhemeryck mhemeryck added the bug Something isn't working label Jun 7, 2020
@mhemeryck mhemeryck self-assigned this Jun 7, 2020
mhemeryck added a commit that referenced this issue Jun 7, 2020
When trying to do updates for coils in using the `CoilGroup.Update`, it
was assumed that the results from the modbus client were returned as an
array of `uint16`. However, they are in fact arrays of `byte`, so the
conversion scheme that was part of `CoilGroup.Update` was NOK and thus
was wrong in case of setups with more of 8 consecutive coils (including
my own).
mhemeryck added a commit that referenced this issue Jun 7, 2020
When trying to do updates for coils in using the `CoilGroup.Update`, it
was assumed that the results from the modbus client were returned as an
array of `uint16`. However, they are in fact arrays of `byte`, so the
conversion scheme that was part of `CoilGroup.Update` was NOK and thus
was wrong in case of setups with more of 8 consecutive coils (including
my own).
mhemeryck added a commit that referenced this issue Jun 8, 2020
When trying to do updates for coils in using the `CoilGroup.Update`, it
was assumed that the results from the modbus client were returned as an
array of `uint16`. However, they are in fact arrays of `byte`, so the
conversion scheme that was part of `CoilGroup.Update` was NOK and thus
was wrong in case of setups with more of 8 consecutive coils (including
my own).
mhemeryck added a commit that referenced this issue Jun 8, 2020
When trying to do updates for coils in using the `CoilGroup.Update`, it
was assumed that the results from the modbus client were returned as an
array of `uint16`. However, they are in fact arrays of `byte`, so the
conversion scheme that was part of `CoilGroup.Update` was NOK and thus
was wrong in case of setups with more of 8 consecutive coils (including
my own).

Spoof a new commit
mhemeryck added a commit that referenced this issue Jun 8, 2020
When trying to do updates for coils in using the `CoilGroup.Update`, it
was assumed that the results from the modbus client were returned as an
array of `uint16`. However, they are in fact arrays of `byte`, so the
conversion scheme that was part of `CoilGroup.Update` was NOK and thus
was wrong in case of setups with more of 8 consecutive coils (including
my own).
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants