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

Examples use double on Arduino side, format 'f' on python side #50

Open
PaulBouchier opened this issue Feb 5, 2022 · 9 comments
Open
Assignees
Labels
question Further information is requested

Comments

@PaulBouchier
Copy link

PaulBouchier commented Feb 5, 2022

Hey PowerBroker - nice library! Thanks very much - it will save me much work compared with PacketSerial.

The last checkin comment for examples/datum/Arduino/ was that it fixed float precision. But Arduino declares doubles in the datum and data examples, but the python side reads it as 'f' (float). Double size is apparently platform-dependent; ghe Arduino language reference says double is 4 bytes (same as float) on Uno and ATMEGA but 8 bytes on Due. I am testing it on an esp32, where apparently doubles are 8 bytes, because I had to change the format specifier in the python example code to 'd' to get it to read the data from esp32-Arduino correctly.

I would think you should make the types match on the Arduino & python sides. Is there some deeper issue here that made you declare Due unsupported? Would such an issue also apply to esp32?

And on the subject of esp32, does the design take care to keep tx & rx logic in Arduino & python (and everything they use) separate such that a receive thread and a transmit thread could run through the classes simultaneously and not step on each other?

@PaulBouchier
Copy link
Author

PaulBouchier commented Feb 5, 2022

One other issue with the examples: the rx examples in python & Arduino loop continuously over if(link.available()), causing the program to consume 100% of the CPU. Inserting sleep(0.01) on the python rx_datum.py example dropped CPU use to 1%. Probably less of an issue on Arduino unless you're running multiple threads. The docs do say it's non-blocking, so the user should know that will happen, but it would be nice if it were better behaved. In fact, only reads are non-blocking - writes on Arduino will block in port->write() once the buffer fills.

@PowerBroker2 PowerBroker2 self-assigned this Feb 5, 2022
@PowerBroker2 PowerBroker2 added the question Further information is requested label Feb 5, 2022
@PowerBroker2
Copy link
Owner

nice library! Thanks very much - it will save me much work compared with PacketSerial.

Thanks! I'm glad you find it useful!

I would think you should make the types match on the Arduino & python sides.

I don't think it's possible to force that compatibility since types such as float32_t don't exist for C++, but if you have ideas on how to do this, I'd be interested in hearing them.

Is there some deeper issue here that made you declare Due unsupported?

I vaguely remember declaring that - I think it was because the Due uses a SPI API that is different than the normal Arduino SPI API. I more or less disabled SPI for this library, so using UART/I2C with SerialTransfer.h shouldn't be an issue for the Due. I you really want, you can re-enable it by uncommenting out the contents of the respective library files. Also, the Due really really sucks - I would strongly suggest using a Teensy 3.X, Teensy 4.X, or ESP32 instead.

And on the subject of esp32, does the design take care to keep tx & rx logic in Arduino & python (and everything they use) separate such that a receive thread and a transmit thread could run through the classes simultaneously and not step on each other?

Transmitting and parsing are handled separately in the library.

The docs do say it's non-blocking, so the user should know that will happen, but it would be nice if it were better behaved.

Both libraries are non-blocking. Misuse of the library is the fault of the user, not the library.

@PaulBouchier
Copy link
Author

Thanks for the answers PowerBroker.

No particularly good ideas about handling platform-specific size of float or double, but float is commonly 4 bytes (but no guarantee). So I would recommend declaring y in the examples as float, for better cross-platform luck. And maybe putting a warning in the docs about platform-specific fundamental type size variations.

I think this library covers all the ground that one just wants when doing a serial link that I think (worry, perhaps) that you'll find it used on an increasing range of combinations of platforms, and people asking, "I've got X talking to Y and Z doesn't work - why?" I've seen it for years on rosserial, and it continues to this day, 10 years down the road. In fact I'm going to be using this in preference to rosserial. The serial side looks more robust in the sense that it is more datagram-like and it's ok to drop packets and that should be handled at higher levels of the design, whereas rosserial is more stream-like, and therefore fragile.

If I may make one more request, it's for API documentation. Looks like you documented pySerialTransfer in a flavor of sphinx autodocs, and I just spend an hour trying to figure out how to use sphinx, unsuccessfully. SerialTransfer has documentation in the .h & .cpp but in an informal format - doxygen is standard for C++ - looks like it should be easy to convert the C++ to be doxygen-compatible and to autogenerate API docs. API docs are so much better than digging through .h & .cpp & .py files to find the types and calling options.

Questions answered, thanks, closing issue.

@PaulBouchier
Copy link
Author

Re-opening, as the object length issue is more pervasive, between Arduino/C++ and pySerialTransfer (and I think between Arduino & Arduino). The API requires specifying the length of strings to receive on the python side, but normally you don't know in advance the length of a string coming from Arduino - for example log messages.

A possible user-level solution is to send 2 objects: an int containing the string length, and then the string. The python receiver client could then receive the length, then make the appropriate string rxObj() request based on the string length. This is clumsy and I think it should work better. Do you have any better user-level solutions?

IMHO, Arduino knows the length of a string it sent, and strings are such a common data type that they should be supported better in the protocol. IOW strings should be treated like objects.

A version of this problem applies when transferring structs to python. The rx_data.py example shows the user has to pull the struct members out, one by one, specifying the type of each, whereas on the Arduino side C++ knows the size of the struct and the templated rxObj method pulls the right number of bytes out. (This problem might be harder to solve if trying to address machines with differing endian-ness though.)

Thoughts?

@PowerBroker2
Copy link
Owner

PowerBroker2 commented Feb 6, 2022

The best user solution is to not use strings at all.

Otherwise, it would be best to use the packet ID field to flag the receiver that the payload is a string (which packet ID corresponds to a string payload is up to the programmer to decide beforehand). Then, the receiver can simply use the payload length attribute to know how many bytes are in the string. That's assuming the string is 254 bytes long or less. Anything more than that, you may need multiple packets per string.

This is by no means impossible - this example (TX and RX) shows how you can transmit a 1KB text file over UART with SerialTransfer.h, but it's more complex than having short strings.

Again, the optimal solution is to not use strings.

Most of the issues you're bringing up, while valid, are incumbent on the programmer. This library is very flexible and powerful, but it's not designed to do absolutely everything for you (especially when interfacing with "weakly-typed" Python). You might have to do some creative thinking and make your own design choices.

@PaulBouchier
Copy link
Author

PaulBouchier commented Feb 7, 2022

Thanks for the reply PowerBroker.

The domain in which the combination of SerialTransfer and pySerialTransfer will find application is embedded devices talking to PCs or like powerful devices which likely have networking and higher-level services. That's what I'm doing: esp32 is running realtime robot control SW, and the PC is running ROS for higher-level autonomy. My specifics don't matter, but I think that embedded <--> PC use case will be common. In that context, it is very valuable to have embedded logs to out to the PC for storage and interpretation as interspersed with higher-level logs. Therefore I think your proposal to not use strings (in the broader sense of variable-length arrays of chars) is a non-starter.

I agree that your library is flexible and powerful, and I was starting down the path of creating my own based on the SerialTransfer library, which is really just a COBS encoding/decoding library - you have to wrap everything else around it - when someone pointed me to your library. Your library offers a ready-built solution, so is attractive. I think what we're discussing here is the appropriate assignment of responsibility to the transport layer vs the application layer. Your library is firmly in the transport layer, and you should fight to keep it there - I'm not trying to change that. But the question is, what should the responsibilities of the transport layer be?

I want to offer my take on the responsibilities of the transport layer, as represented by the following header that I had put in front of my prototyped transport layer:
struct PacketHeader { uint32_t crc; // crc32 over remaining bytes uint8_t pktType; // packet ID, identifies contained data uint8_t pktLen; // packet length, max 255 bytes } __attribute__((packed));

Your implementation provides pktType as packetID - what sort of message is being sent - and crc, but does not offer pktLen. IMHO it should be a responsibility of the transport to support variable-length packets, and to convey enough information for the receiver to know how much data is there. (Think strings/char arrays, or broadly, variable-sized data blobs, or varying numbers of blobs in a packet). It arguably should also be a responsibility of the transport layer to facilitate detection of dropped messages (e.g. by including a sequence #) - I didn't have that in my prototype.

It should be a responsibility of the application layer to understand the messages sent/received and pack/unpack them in a message-specific way. I'd envisioned this kind of class structure for messages:

class PacketType(IntEnum):
    motionRqstMsg = 1
    odomValMsg = 2

class PacketHeader:
    def __init__(self, t=0, l=0):
        self.crc = 0
        self.pktType = t
        self.pktLen = l

    def pack(self):
        print(self.crc)
        s = struct.pack('IBB', self.crc, self.pktType, self.pktLen)
        return s

class MotionRqst:
    def __init__(self, s=0, r=0, b=False):
        self.speed_mps = s
        self.radius = r
        self.blade = b

    def pack(self):
        s = struct.pack('ff?', self.speed_mps, self.radius, self.blade)
        return s

For this to work, the message classes needed access to the byte stream. However, I've come to understand your design as being that the transport layer should hide data encapsulation, and the client should know what elements are in each type of packet, and should pull higher-level things like ints and floats from the transport layer.

In my ideal world, I'd have you add pktLen to your packet, and expose it to RX clients on each end, or at a minimum, allow clients to query whether there is enough data to populate another item of some type T.

I now agree with your design that clients should be responsible for calling the transport API to fish out varying kinds of things from the byte stream (as currently implemented). That would encapsulate the encoding in the transport layer. But for sure, variable length data should be supported.

Thoughts?

@PowerBroker2
Copy link
Owner

Sorry for the delay, I'm pretty busy on weekdays.

In my ideal world, I'd have you add pktLen to your packet, and expose it to RX clients on each end, or at a minimum, allow clients to query whether there is enough data to populate another item of some type T.

If you look at the packet anatomy, I've already included pktLen as # of payload bytes (4th byte of the packet) and is visible by RX and TX sides via the library. This # of payload bytes is accessible through both SerialTransfer.h and pySerialTransfer. On the Arduino side, you can do myTransfer.bytesRead and on the Python side you can do txfer.bytesRead.

I hope this helps

@PaulBouchier
Copy link
Author

Ahh - I missed that - thank you - that's perfect! It will make variable-length string transfers easy.

@PaulBouchier
Copy link
Author

@PowerBroker2 I'm looking at what it takes to fix the issue in the examples to do with double and float.
Between release 2.1.5 and 2.1.6 the only change was to change
float y
to
double y
Why did you do that? float and double are the same size on Arduino Uno and other AVR-based boards. Was there some other compatibility issue you were fixing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants