jfjlaros / simpleRPC

Simple RPC implementation for Arduino.
MIT License
50 stars 17 forks source link

Allow Ethernet Connections (arduino) #11

Closed chrisflesher closed 3 years ago

chrisflesher commented 3 years ago

Pull Request Details

This is the arduino side needed to support Ethernet and WiFi101 connections.

Breaking Changes

There are no breaking changes, this PR just adds an example .ino files to add Ethernet and WiFi support. I've only tested the wifi101 demo, I think the ethernet example is so similar to the WiFi it should work but haven't tested yet.

Other Relevant Information

Added matching code to jfjlaros/arduino-simple-rpc PR #4. The basic strategy is to use the pyserial serial_for_url function call instead of Serial constructor. This allows you to set the device name to be something like socket://192.168.1.151:10000 instead of /dev/ttyACM0. I was able to get WiFi working using the command simple_rpc list -d socket://192.168.1.151:10000

chrisflesher commented 3 years ago

There still seems to be some issue with this approach. Getting the methods seems to work fine. However when I call simple_rpc call ping 1 -d socket://192.168.1.50:10000 I would expect it to return a 1, instead it returns 0. I found that the python side is writing a 1 (on line 166 of simple_rpc.py) and that the Arduino ping function receives a 0... so maybe something in interface.tcc is not processing the arguments properly? Any ideas?

jfjlaros commented 3 years ago

Maybe it is a timing issue? Do you know whether the Stream read and write calls are blocking or non-blocking?

chrisflesher commented 3 years ago

I think they are usually non-blocking. For my work we often use rs485 connections and have to use a flush and microsecond delay to get transmission to work sometimes.

On Sun, Nov 29, 2020, 6:04 AM Jeroen F.J. Laros notifications@github.com wrote:

Maybe it is a timing issue? Do you know whether the Stream read and write calls are blocking or non-blocking?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jfjlaros/simpleRPC/pull/11#issuecomment-735383220, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFJ2UI64J7PDNOAIXRXL3SSI2F3ANCNFSM4UFPT32Q .

jfjlaros commented 3 years ago

I only had blocking I/O in mind when writing this library, using non-blocking I/O will probably break functionality at higher speeds.

Perhaps setting an explicit timeout with Stream::setTimeout() is worth a try.

chrisflesher commented 3 years ago

Hmm... I would have thought Serial would be non-blocking as well...

https://www.arduino.cc/reference/en/language/functions/communication/serial/flush/

On Sun, Nov 29, 2020 at 6:41 AM Jeroen F.J. Laros notifications@github.com wrote:

I only had blocking I/O in mind when writing this library, using non-blocking I/O will probably break functionality at higher speeds.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jfjlaros/simpleRPC/pull/11#issuecomment-735387736, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFJ2TRTOYJYFL3USTCVELSSI6OZANCNFSM4UFPT32Q .

chrisflesher commented 3 years ago

I tried adding a io.flush() to rpcPrint and rpcWrite, it didn't help, still debugging...

chrisflesher commented 3 years ago

Here is a log of the bytes I am getting with the function simple_rpc call ping 1 -d socket://192.168.1.50:10000:

rpcRead: 00 
rpcRead: 00 
rpcWrite: 00 
rpcRead: 01 
rpcRead: 00 20001101 00 20001188 
rpcWrite: 01 01 00 06 

The python side says it is writing ff 00 01 but the arduino side says it is reading ff 00 00 01, there is an extra 00 somehow?

Here is the log for simple_rpc call inc 100 -d socket://192.168.1.50:10000:

rpcRead: 01 
rpcRead: 00 20001101 00 20000240 
rpcWrite: 01 01 00 06 
rpcRead: 64 
rpcRead: 00 
rpcRead: 00 
rpcWrite: 00 
rpcRead: 00 
rpcRead: 00 
rpcWrite: 00 
jfjlaros commented 3 years ago

Which sketch are you using? And does this sketch work when using the serial interface?

chrisflesher commented 3 years ago

Am using the wifi101 sketch with some extra Serial.print and io.flush statements in the .tcc files not shown in this pr for debugging.

On Sun, Nov 29, 2020, 3:13 PM Jeroen F.J. Laros notifications@github.com wrote:

Which sketch are you using?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jfjlaros/simpleRPC/pull/11#issuecomment-735454733, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKFJ2U7UNMCNPJWPXAUAYLSSK2QXANCNFSM4UFPT32Q .

jfjlaros commented 3 years ago

This is the output I get when doing more or less the same thing using the serial interface:

>>> from serial import Serial
>>> c = Serial('/dev/ttyACM0')
>>> c.write(bytes([0, 1]))
2
>>> c.read()
b'\x01'
>>> c.read()

The last instruction blocks because there is no more data.

Could you share the output of a similar exercise for the ethernet interface?

chrisflesher commented 3 years ago

Getting the same thing (both manually and run as a script):

>>> from serial import serial_for_url
>>> c = serial_for_url('socket://192.168.1.50:10000')
>>> c.write(bytes([0, 1]))
2
>>> print(c.read())
b'\x01'
>>> print(c.read())
chrisflesher commented 3 years ago

However the same example split into two individual writes seems to fail (this works when using serial):

>>> from serial import serial_for_url
>>> c = serial_for_url('socket://192.168.1.50:10000')
>>> c.write(bytes([0]))
1
>>> c.write(bytes([1]))
1
>>> print(c.read())
b'\x01'
>>> print(c.read())
b'\x02'

The returned bytes seem to be generated non-deterministically, they are not always b'\x01' b'\x02', sometimes they are b'I' b'J'

jfjlaros commented 3 years ago

What happens if you start reading immediately? E.g.,

>>> from serial import serial_for_url
>>> c = serial_for_url('socket://192.168.1.50:10000')
>>> print(c.read())

If this and successive reads return anything, my guess would be that read() is nonblocking.

jfjlaros commented 3 years ago

As a test, could you add the following line to the top of StreamIO::read in plugins/stream/io.cpp and repeat your test with two individual writes?

while (!available());
chrisflesher commented 3 years ago

With this example

>>> from serial import serial_for_url
>>> c = serial_for_url('socket://192.168.1.50:10000')
>>> print(c.read())

It waits indefinitely.

I couldn't test the plugins/stream/io.cpp change because I'm passing a WiFiClient objejct directly.

I was able to get it working by changing the rpcRead function to be blocking:

template <class I, class T>
void rpcRead(I& io, T* data) {
  while (io.available() < sizeof(T));
  io.read((uint8_t*)data, sizeof(T));
}

I'm not sure if this is a great idea long term? It allows the WiFi connection to work properly though. Otherwise the WiFiClient.read() seems to return randomly generated bytes.

jfjlaros commented 3 years ago

Good to hear that you found a workaround. I agree that this might not be a long term solution, so could you also try this fix in plugins/stream/io.cpp?

void StreamIO::begin(Stream& ss) {
  _ss = &ss;
  _ss->setTimeout(1000);
}

If I understand it correctly, this should make the read blocking but it will give up after 1 second.

chrisflesher commented 3 years ago

Ok, here is a new suggestion. I found the Arduino Stream class already has a default timeout of 1 second and we can use the readBytes function to implement the timeout. This was working before for Serial because it was already using readBytes under the hood (in plugins/StreamIO/io.cpp). But wasn't working for WiFiClient because it was using a read function that didn't implement a timeout.

My suggestion is to rename all io.read calls to io.readBytes, this allows you to use Stream objects directly with the interface function call instead of going through the plugin wrapper and take advantage of the timeout. I think this approach simplifies the code slightly but maybe there is something I'm not envisioning with the plugins, if you want to keep the plugin style of doing things we could wrap the WiFiClient in the StreamIO plugin.

jfjlaros commented 3 years ago

Yes, that sounds like a good solution.

So now we can simply pass either Serial or WiFiClient to the interface function, making the whole plugin structure redundant?

jfjlaros commented 3 years ago

Great, that would save quite some code and template magic.

I guess that if we do need a wrapper for some other protocol, we can make it a subclass of Stream as well.

chrisflesher commented 3 years ago

Well I tried running the Serial demo and it didn't work, will try to look at it some more...

chrisflesher commented 3 years ago

Ok, I tested passing both Serial and WiFiClient directly to interface and they both seem to work now. Think this is ready for more review or merge.

jfjlaros commented 3 years ago

Thank you, I confirmed this works for me as well. Nice work.

I will update the test fixture before I will do the merge.