piControl: use serdev based revpi-comm device for RS485 communication

[linosanfilippo-kunbus]

There is a serdev implementation for the RS485 piBridge-communication available. Use that instead of establishing an own serial connection in piControl. For now we only replace the low-level functions with pibridge_send() and pibridge_recv_timeout() to keep code changes minimal. An exception is the use of pibridge_req_gate_tmt() which covers a hole transaction (request and reception of data with consecutive check of received data; in a later step only high-level functions that cover complete transactions should be used).

Clean up the code and remove everything that is not needed any longer, especially the UART thread and the related data structures which are now replaced by the functionality implemented in the serdev module.

Signed-off-by: Lino Sanfilippo l.sanfilippo@kunbus.com

[l1k]

This comment has not been addressed AFAICS:

There's a slight change of behavior introduced in `piIoComm_sendRS485Tel()`: The original version trusted the length of the received data as given in the received header. It received exactly the number of bytes reported there. The new version only receives the number of bytes asked for by the caller. It also calculates the CRC8 only for the number of bytes given by the caller. So if the received packet differs in size, the CRC8 calculation will be wrong. It seems safer to receive the number of bytes specified in the received header. If the caller requested fewer bytes, return only those fewer bytes. If the caller requested more bytes than were received, return an error.

The risk I see here is that the module may send more data than expected and indicates so in the header of the telegram it sends. If we blindly trust our own expectation of the length, we may e.g. send another telegram while the module is still transmitting its response. Then both the module and the Core/Connect would transmit simultaneously on the bus, which is forbidden. So I think we do need to receive the whole telegram before carrying on. Of course we should return an error on mismatch of expected and actual received length.

[linosanfilippo-kunbus]

This comment has not been addressed AFAICS:

There's a slight change of behavior introduced in piIoComm_sendRS485Tel(): The original version trusted the length of the received data as given in the received header. It received exactly the number of bytes reported there. The new version only receives the number of bytes asked for by the caller. It also calculates the CRC8 only for the number of bytes given by the caller. So if the received packet differs in size, the CRC8 calculation will be wrong. It seems safer to receive the number of bytes specified in the received header. If the caller requested fewer bytes, return only those fewer bytes. If the caller requested more bytes than were received, return an error.

The risk I see here is that the module may send more data than expected and indicates so in the header of the telegram it sends. If we blindly trust our own expectation of the length, we may e.g. send another telegram while the module is still transmitting its response. Then both the module and the Core/Connect would transmit simultaneously on the bus, which is forbidden. So I think we do need to receive the whole telegram before carrying on. Of course we should return an error on mismatch of expected and actual received length.

This is indeed a problem: how can we manage to receive the whole datagram if the user provided a buffer that is too short? I think the correct approach here would be to check the received header , allocate internally a buffer for the expected data length (as announced in the header), receive all data in the allocated buffer and then copy all of it (or as much as possible) to the buffer provided by the caller.

[l1k]

I think the correct approach here would be to check the received header , allocate internally a buffer for the expected data length (as announced in the header), receive all data in the allocated buffer and then copy all of it (or as much as possible) to the buffer provided by the caller.

I think that's what the code does historically. But copying data around sucks performance-wise. I'd suggest to receive directly into the buffer provided by the caller, but insert a check whether we're already past the buffer's end. Alternatively, if the received and expected length mismatch, consume the received bytes but do not store them to the buffer and return an error. It's probably not worth it returning partial responses to the caller.

[linosanfilippo-kunbus]

I think the correct approach here would be to check the received header , allocate internally a buffer for the expected data length (as announced in the header), receive all data in the allocated buffer and then copy all of it (or as much as possible) to the buffer provided by the caller.

I think that's what the code does historically. But copying data around sucks performance-wise. I'd suggest to receive directly into the buffer provided by the caller, but insert a check whether we're already past the buffer's end. Alternatively, if the received and expected length mismatch, consume the received bytes but do not store them to the buffer and return an error. It's probably not worth it returning partial responses to the caller.

I doubt that it makes a noticeable difference if we copy directly to the buffer or copy the few bytes from an allocated buffer to the destination buffer. Especially since our loop times are in msecs range and thus order of magnitudes higher than the time it takes to copy a few data bytes (how long might it take, a few usecs?). I would like to keep the implementation simple until we see that we really have to micro optimize.

[l1k]

Well you can detect early on if the size mismatches. Then just consume number of bytes equal to that size and return an error. Otherwise receive into the caller's buffer. Sounds simple enough to me?

[linosanfilippo-kunbus]

But if the provided buffer is too small we have to use some kind of 'discard buffer' to copy the rest of the datagram into, to make sure that the whole datagram is consumed, right? Thats what seems a bit ugly to me.

[l1k]

Add a pibridge_skip() API function to pibridge.c (in the kernel PR) which takes the number of received bytes to discard. Model that API function after pibridge_recv_timeout() except it calls kfifo_skip() this number of times (instead of kfifo_out()). Whenever there's a length mismatch, call pibridge_skip() to discard the subsequently received data (instead of pibridge_recv_timeout()).

Alternatively, wait until the telegram has been received in full, then call pibridge_clear_fifo().