NI Modbus API on GitHub

[Porter]

NI Labs Modbus API is now on GitHub!

Password protection of VIs has been removed.

https://github.com/NISystemsEngineering/LabVIEW-Modbus-API

[smithd]

yay!

please don't look inside some of the code is terrible

 

 

In all seriousness, I think a lot of the class stuff was overkill for what the code needed to do, but most of it doesn't cause a problem. The biggest issue is the (positive) feedback I posted on yours -- I did the serial stuff wrong, in my opinion, by trying to follow the spec rather than doing it the more pragmatic way (looking at the function code and parsing out the length manually). A series of leaky abstractions all the way from linux-rt up through visa and into the serial layer in that library led to a very hacky workaround on the serial side, and the workaround could have been avoided if I had been more pragmatic at the time. It does work, but its definitely less than ideal. The significant area where your version and this version differ is your attempt to make the object thread safe which...doesn't hurt, but I'm still not sold on. Maybe its just how I write applications, but I can't think of any situation where I'd want to share the port between loops. I thought I had a use not too long ago but since the port is multidrop anyway I just serialized everything.

Edited August 17, 2018 by smithd

[Neil Pate]

I love the comment in the CRC calculation VI 

But seriously I would way rather have a not perfect, but functional, implementation any day over password protected stuff. In particular when I was new to modbus and trying to understand which end was actually the tcpip server and how data was persisted this would have been great to have. 

Edited August 18, 2018 by Neil Pate

[smithd]

Well to be fair, there are a lot of variants: https://en.wikipedia.org/wiki/Cyclic_redundancy_check#Polynomial_representations_of_cyclic_redundancy_checks

The comment was intended more in the sense of "I can't be assed to figure out which variant modbus uses when I have a functional implementation right over here"

 

[Porter]

On 8/17/2018 at 6:19 PM, smithd said:

The significant area where your version and this version differ is your attempt to make the object thread safe which...doesn't hurt, but I'm still not sold on. Maybe its just how I write applications, but I can't think of any situation where I'd want to share the port between loops. I thought I had a use not too long ago but since the port is multidrop anyway I just serialized everything.

At the time, I was using the actor framework for a large project. I had multiple com ports and multiple devices on each com port. I decided to have an actor per device. Each modbus device actor would build its own modbus instance from the device's configuration file. Since the modbus instances were thread safe, I didn't have to worry about sharing a com port with multiple devices. Devices on the same com port simply wait in a FIFO queue for the port to be available.

I could have instead implemented a modbus master actor and spun up one of these actors per com port. The device actors would then have to send/receive their modbus data through the modbus master actor running on their com port. This way, the modbus master library doesn't need to be thread safe (in terms of shared com port), but I found that it added an extra layer of complexity. I prefer to handle serial communication in my hardware abstraction layer rather than my application layer. Otherwise the device's driver would be tied to the actor framework and would be difficult to reuse for other projects

Edited August 20, 2018 by Porter

[ShaunR]

On 8/17/2018 at 10:19 PM, smithd said:

but I can't think of any situation where I'd want to share the port between loops

I can. If you use event driven comms then you need it.

Ala:

 

[smithd]

On 8/20/2018 at 7:02 AM, Porter said:

At the time, I was using the actor framework for a large project. I had multiple com ports and multiple devices on each com port. I decided to have an actor per device. Each modbus device actor would build its own modbus instance from the device's configuration file. Since the modbus instances were thread safe, I didn't have to worry about sharing a com port with multiple devices. Devices on the same com port simply wait in a FIFO queue for the port to be available

I got back to looking at the code today, I had forgotten but it looks like I did make the object thread-safe. All of the request-response calls are mutexed.

 

For giggles, I spent some time on it today and just pushed a few changes to a fork here: https://github.com/smithed/LabVIEW-Modbus-API

I didn't like how my old team did the transaction id fix, so thats one commit. The second commit is to fix the serial thing so the code is no longer nasty. For ascii I added a method to 'read until CRLF' to the network class. For RTU I added a cheat method which tries to guess how big the packet is. If the packet is unknown size, it risks a CRC collision but just polls whatever is at the port. It totally ignores the 3.5 char times nonsense now. I tested all 3 standard pairs on localhost, with the serial ports emulated using com0com. This isn't a perfect representation of real life, but it works ok.

[Porter]

On 11/30/2018 at 1:28 AM, smithd said:

I got back to looking at the code today, I had forgotten but it looks like I did make the object thread-safe. All of the request-response calls are mutexed.

It is thread safe as long as you use the same instance of serial master when communicating with multiple slaves on the same bus.

I finally took a look at the code. I have a couple comments/questions:

Serial Shared Components.lvlib->Configure Serial Port.vi: Case structure for number of stop bits. I suggest having some override for this. Maybe have a property node for the stop bits setting. I've come across a number of situations where I have needed RTU with no parity and one stop bit.

RTU Data Unit.lvclass->Calculate CRC.vi: I think that there is a more efficient way to calculate the CRC using a lookup table. I'd be happy to share it when I get around to implementing it on Plasmionique Modbus Master.

Serial Shared Components.lvlib->Serial Read.vi: I don't like the idea of polling the bytes at port every 8ms. Why not just read the specified number of bytes and let VISA handle the timeout?

Serial Shared Components->Serial Read to CRLF.vi: Why not just read until LF (let VISA read take care of this)?

ASCII Data Unit.lvclass->Read ADU Packet.vi: Why is start character written to request unit ID of Serial Data Unit? Shouldn't it be the unit ID?

IP Data Unit.lvclass->Read ADU Packet.vi: Transaction ID mismatch will discard the packet. What will happen on a noisy network connection with multiple transactions being sent out? See: https://github.com/rfporter/Modbus-Master/issues/1

Why does TCP Master/Slave need Protocol Read to CRLF.vi?

TCP_NODELAY.vi: Cool... Was this ever used & tested?

[smithd]

12 hours ago, Porter said:

TCP_NODELAY.vi: Cool... Was this ever used & tested?

I dont think so, I forgot it was in there. Seems like a good idea though. Just needs to be added to TCP Master::Initialize Master and TCP Slave::wait on listener.

12 hours ago, Porter said:

Serial Shared Components.lvlib->Configure Serial Port.vi: Case structure for number of stop bits. I suggest having some override for this. Maybe have a property node for the stop bits setting. I've come across a number of situations where I have needed RTU with no parity and one stop bit.

This is illegal per the spec. Its obviously not difficult to change this, but...its also not hard just to set the value after initializing the modbus library.

12 hours ago, Porter said:

RTU Data Unit.lvclass->Calculate CRC.vi: I think that there is a more efficient way to calculate the CRC using a lookup table. I'd be happy to share it when I get around to implementing it on Plasmionique Modbus Master.

I think I see what you mean and attached an implementation. Looks to be about 3x faster (edit: 2x with debug off) to read from the lookup vs calculating it out.

Just a thought tho, it probably makes other code around it slower by completely trashing your CPU cache (its about 1/4 of the L2 cache on a zynq-based cRIO).

12 hours ago, Porter said:

Serial Shared Components.lvlib->Serial Read.vi: I don't like the idea of polling the bytes at port every 8ms. Why not just read the specified number of bytes and let VISA handle the timeout?

If visa times out it returns whatever is in the buffer. If it happens to time out mid-packet (as may be the case on linux rt or with usb-serial adapters) then you have half the packet in your read loop and half out on the bus. This isn't important for the master, since if you time out you pretty much have to flush the buffer, but for the slave its sitting there waiting for data forever, so dealing with partial packets is annoying.

Also, this was like 6 years ago so I may be wrong, but at the time I think one of my goals was to make the serial and tcp functions act the same. Thats why TCP also uses the 'buffered' feature.

12 hours ago, Porter said:

Serial Shared Components->Serial Read to CRLF.vi: Why not just read until LF (let VISA read take care of this)?

I do, thats why I make sure to enable the term char. However it has to be CRLF, LF by itself is not acceptable. You may very well ask in which situation you would get a LF by itself -- I don't know, but I do know that its a 5c chip wiggling the voltage on some wires back and forth at an absurd speed, so I figure it can't hurt to check 

12 hours ago, Porter said:

ASCII Data Unit.lvclass->Read ADU Packet.vi: Why is start character written to request unit ID of Serial Data Unit? Shouldn't it be the unit ID?

Woops. That method really only exists to make sure its not broadcast, so in the 99% use case it happens to work. Otherwise ascii doesn't care.

Fixed, though.

12 hours ago, Porter said:

IP Data Unit.lvclass->Read ADU Packet.vi: Transaction ID mismatch will discard the packet. What will happen on a noisy network connection with multiple transactions being sent out? See: https://github.com/rfporter/Modbus-Master/issues/1

I don't actually understand what the problem was with this. As you said in your comment on that issue, each access should be synchronous for a given master or slave, so there is no such thing as multiple transactions outstanding on the connection. The transaction ID check just sort of verifies that. If an error occurs, you must close the connection and reset. I can't think of any reason that would not be the right response, can you? I'm also confused by this because that part of the code was implemented by Tanner, who is the person who posted that issue to yours, so presumably he thought that code fixed the issue?

Note that this behavior (close and reopen) is different from serial (wait, flush the buffers, and hope things start to work again), not because the serial way is better but because serial is a 5c chip twiddling the voltage on some wires. The serial version has no connection to close.

12 hours ago, Porter said:

Why does TCP Master/Slave need Protocol Read to CRLF.vi?

The idea of the pluggable transport was to support any modbus adu over any network type. I've definitely heard of RTU over TCP, this would be to support ascii over tcp

the real answer is that it was easy and it kept the implementations mirrored.

crc bench.vi

crc.vi

Edited December 7, 2018 by smithd

[Porter]

17 hours ago, smithd said:

I think I see what you mean and attached an implementation. Looks to be about 3x faster (edit: 2x with debug off) to read from the lookup vs calculating it out.

Just a thought tho, it probably makes other code around it slower by completely trashing your CPU cache (its about 1/4 of the L2 cache on a zynq-based cRIO).

For comparison, I copied the CRC calculation from the modbus spec. It's slightly faster (when run on my computer) than your method. I'm not exactly sure why though. And it uses less memory (512Bytes for LUT instead of 131kBytes).

 

crc bench.vi

CRC16_Modbus.vi

[Porter]

18 hours ago, smithd said:

I do, thats why I make sure to enable the term char.

Hah. I didn't see that. I was looking somewhere else. But anyway, I don't see a case where you would only get a LF. In the ASCII protocol, a lone LF should never be sent within the message. And if, for whatever reason, a bit flips to make an LF, then hopefully the error would be caught by failing the CRC check.

[Porter]

18 hours ago, smithd said:

I don't actually understand what the problem was with this. As you said in your comment on that issue, each access should be synchronous for a given master or slave, so there is no such thing as multiple transactions outstanding on the connection. The transaction ID check just sort of verifies that. If an error occurs, you must close the connection and reset. I can't think of any reason that would not be the right response, can you? I'm also confused by this because that part of the code was implemented by Tanner, who is the person who posted that issue to yours, so presumably he thought that code fixed the issue?

Note that this behavior (close and reopen) is different from serial (wait, flush the buffers, and hope things start to work again), not because the serial way is better but because serial is a 5c chip twiddling the voltage on some wires. The serial version has no connection to close.

His code has not fixed the issue yet. It does catch the error, as did Plasmionique Modbus Master, but it is up to you to catch the error and reset the connection. If you don't reset the connection, this can happen:

Yikes! No errors being reported and read register 0 is returning the value for register 1.

Tanner's proposed fix works. I implemented it in the Plasmionique Modbus Master. With it you can have multiple pending transactions. The responses will be matched up with the requests using the transaction ID.

TCP Test.vi

[smithd]

2 hours ago, Porter said:

For comparison, I copied the CRC calculation from the modbus spec. It's slightly faster (when run on my computer) than your method. I'm not exactly sure why though. And it uses less memory (512Bytes for LUT instead of 131kBytes).

Ah yes thats much nicer. I didn't know it could be broken up like that

Edited December 7, 2018 by smithd

[smithd]

1 hour ago, Porter said:

it is up to you to catch the error and reset the connection. If you don't reset the connection, this can happen:

Lol sounds like intended behavior to punish those who ignore errors 

I think the missing part is that Tanner also added an incrementing counter on the send side, so you should see errors on every subsequent read because the counter will never catch up.

Edited December 7, 2018 by smithd

[Porter]

It should show an error on every subsequent read... but it doesn't:

It just keeps going as if nothing is wrong.

Edited December 10, 2018 by Porter