[Modbusino] - simple modbus rtu library

[kmihaylov]

This PR introduces Modbusino. Component and sample with modifications for Sming.

The most important modification is the replacement of Serial.flush() with Serial.clear().
I removed some code that was spamming the line in case of MB errors.
I also added some minimal delays for the interface IC to ensure proper line detection when transmitting data.

Four environment variables exist for the component and one config variable is for the application (the modbus address).

[mikee47]

My main concern about the current implementation is the use of polling. Whilst I dislike that for a MODBUS master device, for slave it's just not on.

Rather than attempting to patch the modbusuino library, I would suggest forking it. As there aren't any complex requirements the whole thing can be addedas a Component submodule.

Essentially the contents of the 'loop' method need to be invoked from a Serial callback. Take a look at the SerialReadingDelegateDemo class in the Basic_Serial sample to see how it can be done.

The ModbusinoSlave should also have a means to register a user callback, which is then invoked when a message is received.

I don't have any modbus slave implementations at all so if you'd like me to take a pop at this I'd be happy.

[mikee47]

I've had a longer think about this and I think to do this properly involves integrating master and slave functionality in the same library as there's a lot of common code involved. Rewriting the modbusino library as I suggested above is probably not the best way forward so happy with this PR as-is, minor points notwithstanding.

[kmihaylov]

Thank you for your corrections, these README files could serve as good templates.

Your opinion about the polling changed my mind about how hard would this to be implemented. If you're not in a hurry I may try these changes (in few weeks probably)?

My idea was to include some stuff while your library is getting prepared. I thought it supports both master and client? Maybe I'm in a mistake!

[mikee47]

When transmitting, you can express the pre/post delays in terms of characters
then use the 'transmit complete' serial callback to set RE inactive again.

This means you don't need to wait around for the transmission to complete,
and you no longer require pre/post delay variables since the delays are now tied to the baud rate.

You could also use this approach in ModbusMaster.

void ModbusinoSlave::setup(long baud)
{
    Serial.begin(baud);

    // When any transmission completes, set RE line inactive
    Serial.onTransmitComplete(
        [](HardwareSerial &) { digitalWrite(RS485_RE_PIN, !RS485_TX_LEVEL); });
}

static void send_msg(uint8_t *msg, uint8_t msg_length)
{
    uint16_t crc = crc16(msg, msg_length);

    msg[msg_length++] = crc >> 8;
    msg[msg_length++] = crc & 0x00FF;

    // Set RE active, add a one-character delay before and after message
    digitalWrite(RS485_RE_PIN, RS485_TX_LEVEL);
    constexpr uint8_t NUL{0};
    Serial.write(NUL);
    Serial.write(msg, msg_length);
    Serial.write(NUL);

    // RE is set inactive by Serial transmit-complete callback
}

[mikee47]

My idea was to include some stuff while your library is getting prepared. I thought it supports both master and client?

#1991 (comment)

[kmihaylov]

OK let me give it a try and I'll try to make a commit today.

[slaff]

Yes.

@mikee47 @kpishere Can you review this PR and test it with a real device?

[kmihaylov]

If boards are needed maybe I can help.

[slaff]

Rename MbSlave to mbSlave to match the coding style.

[slaff]

@mikee47 Any comments here?

[mikee47]

I've tested this using https://sourceforge.net/projects/qmodmaster/ and works fine.

I noted a typical delay of around 5ms before RE goes low after a transmission (about 5 characters at 9600 baud) although this is variable because you're using the task queue callback. Probably not an issue for slave mode, however I note in modbusinoSlave.cpp you've got uartCallback commented-out in (lines 66+ and 96+). If you were having trouble getting this to work note that uartCallback is invoked in interrupt context so line 66 would require IRAM_ATTR adding to the function.

[kmihaylov]

I'll fix these once I get back to the city, because I'm currently locked out. BTW I'm using 115200, probably that is why I didn't notice the delays you have observed. Maybe I can switch to 9600 and try the interrupt.

[mikee47]

It's not actually related to baud rate. Below, the blue trace is RE and red trace is slave response. I see either this:

And sometimes this:

The delay is caused by the mbPrint() callback - commenting it out eliminates the delay. This is because on these occasions the RECEIVE handler is getting called before transmitComplete.

If the master tries to send another command within this 5ms period then it'll get lost, otherwise it may not be an issue.

[kmihaylov]

I note in modbusinoSlave.cpp you've got uartCallback commented-out in (lines 66+ and 96+). If you were having trouble getting this to work note that uartCallback is invoked in interrupt context so line 66 would require IRAM_ATTR adding to the function.

Sure I have. How could I forget IRAM_ATTR ! Just did as you propose, however the interrupt did not fire. Before I had some behavior like the interrupt is called after the transmission of the first NUL character (when I didn't use IRAM_ATTR). Should I force enable interrupts?

[mikee47]

... however the interrupt did not fire.

Because the call to Serial.onDataReceived resets the interrupt callback. For this to work you'd need to handle receives in your ISR as well. Operation seems good enough without this so I'd suggest just removing the commented-out lines.

Before I had some behavior like the interrupt is called after the transmission of the first NUL character (when I didn't use IRAM_ATTR). Should I force enable interrupts?

Interrupt occurs when the transmit FIFO becomes empty. When idle, that will (probably) happen as soon as you write the first character because it gets moved (almost) immediately from the FIFO into the output shift register. A simple way to mitigate this is to set a flag when the message has been written; ignore any TX_EMPTY interrupt unless that flag has been set.

[kmihaylov]

@mikee47 @slaff
I removed the unused setUartCallback function. Also I added a note to the component's readme describing the RE delay. I understand that better is to replace onTransmitComplete with ISR, so maybe the next update would do so...
Meanwhile I'm sticking with Mike's recommendation to leave it this way.

[slaff]

Meanwhile I'm sticking with Mike's recommendation to leave it this way.

@kmihaylov If the PR is ready in the current form I will schedule it for merging.

[kmihaylov]

Let me compile and flash it just to be sure and I'll write if everything works in 15 minutes.

[kmihaylov]

Yes, it works as expected. I do not observe RE delays on the default conditions for ModbusMaster and modbusino, as they exist now in the PR (115200). I remind that Mike is using another testing approach.

[slaff]

I remind that Mike is using another testing approach.

@mikee47 Do you want to do a final test before I merge this PR?

[mikee47]

@slaff Looks fine, happy to merge.