FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Import Stream RTU by Miq1 · Pull Request #269 · eModbus/eModbus · GitHub

Import Stream RTU - #269

Merged
Miq1 merged 12 commits into
masterfrom
Stream-RTU
Feb 22, 2023
Merged

Import Stream RTU#269
Miq1 merged 12 commits into
masterfrom
Stream-RTU

Conversation

Miq1 commented Feb 16, 2023

Copy link
Copy Markdown
Contributor

Preparing next release

Miq1 requested a review from bertmelis February 16, 2023 14:19

bertmelis commented Feb 17, 2023
edited
Loading

Copy link
Copy Markdown
Contributor

We can still keep the HardwareSerial constructors and cast the hwserial reference inside the constructor.

It's not really necessary for me but the less technical users might benefit from backwards compatability.

EDIT: This remark is completely up to you. I'm totally fine with "leaving it up to the user". I've got no other remarks.

Miq1 commented Feb 17, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the reading and comments. I considered offering the HardwareSerial constructors as well, but then we only can set the FIFO threshold and read out the baud rate, the buffer size remains out of reach. If we aloow that we should limit the baud rates available to 115200 and below, as I found that anything above will bust the buffer with larger messages.

I will think again...

Copy link
Copy Markdown
Contributor

what do you mean with the buffer size remains out of reach. If the user is able to set the buffer, we can do it too, right? Am I missing something here?

Copy link
Copy Markdown
Contributor

Forget my last remark. The user already calls .begin(...). This would only work if our lib calls hwserial.begin(...).

Miq1 commented Feb 19, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

Surprise, surprise.

I modified the RTU sources again to have different constructors for Stream with mandatory baud rate parameter and HardwareSerial without, as I expected to read that by HardwareSerial::baudRate() at run time, and did a setRxFIFOFull(1); there as well.

Did not work. I first got a division by zero reboot loop because the baud rate was zero at the time I called the interval calculation (huh?), but just safeguarded that with a default interval value without thinking further.

And now got lots of errors with RTU...

Reason is, the constructor for the RTU object gets called very early, if the objects are declared global. The HardwareSerial baud rates are determined at the begin() call in setup() - which happens much later. The setRxFIFOFull() fails for the same reason, it is only effective is issued after the serial's begin().

Too bad, it would have been very convenient to only have a different constructor. The begin() call of the ModbusRTU classes could be made to recognize the explicit class of the object (whether it is a HardwareSerial or some other Stream) if written as a function template, but that again will require it to be called like RTUserver.begin<HardwareSerial>(1);, which is no improvement.

The final idea was to give the Stream or HardwareSerial object with the begin() call only and omit it from the constructor. Then we would have something along

SoftwareSerial ss(...);
...
ModbusClientRTU mcr;
...
// Then
mcr.begin(Serial1);
// or
mcr.begin(ssr, 9600);

Would you like that better? It would open the opportunity to switch interfaces at run time, since every new begin() will be able to use a different.

Miq1 commented Feb 20, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

I now have a version with the proposed shift of the Stream object into the begin() call.

I do not know how to judge the result.

Positive:

  • The setRxFIFOFull(1) is integrated into begin(), the user must not care about it
  • switching interfaces or baud rates followed with another begin() is working

Negative:

  • If you will try begin(Serial1, Serial1.baudRate()), i.e., call the Stream-type begin() with a HardwareSerial, you will get errors amass. I tried to catch and redirect such calls by checking the typeid in begin(), but that does require the -frtti build flag I am sure no one will be able to set in the Arduino IDE at least.
  • I cannot prevent the user being responsible to do the setRxBufferSize(260) calls before the Serial.begin(). The problem remains the same: it has to be set before starting the interface due to the core, but stopping it, setting the buffer size and restarting it is not possible for us due to incomplete configuration information available.
  • the buffer problem is symmetrical, by the way. You also will need to call setTxBufferSize(260) to prevent segmented messages the other way round.
  • you will get errors with messages larger than 127 bytes at all baud rates above 9600, if the two buffer resizes have not been done before. Interestingly the number of errors seems not to be increasing significantly with the baud rate - whyever.

So we have two options:

  • urgently ask the users to add the two buffer size calls before their serial begin()
  • Require all serial parameters in the constructors for a HardwareSerial, stop it in case it is running, configure it to our needs and restart.

Neither of both is appealing me much.

Miq1 commented Feb 20, 2023

Copy link
Copy Markdown
Contributor Author

I now created a static utility function RTUutils::prepareHardwareSerial(HardwareSerial& s, uint16_t bufferSize = 260); that will set both buffers to 260 (or another value, if given).

But still we must require the users to call it correctly before the serial begin()...

Miq1 requested a review from bertmelis February 20, 2023 14:58

Copy link
Copy Markdown
Contributor

I'd say that is the cleanest solution (for now?).

Miq1 commented Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for checking. I did not find a better solution and at least it does require less diligence from the users.
I am going to merge the PR now, update the docs tomorrow and publish a release 1.7 with it.

Miq1 merged commit f9f6b6e into master Feb 22, 2023
Miq1 deleted the Stream-RTU branch February 22, 2023 19:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants


Back | FazBrowse Home | New Git URL