add timeout configuration for sync serial reads

[onur-ozkan]

Adds timeout configuration for serial reads.

[onur-ozkan]

Pinging @akumaigorodski so they know their change is updated.

[patrickelectric]

Just to be sure, what was broken ?

[onur-ozkan]

Just to be sure, what was broken ?

recv() can time out after 1 millisecond when there are no messages available. If someone wants non-blocking, one-shot read they can use try_read instead. A 1 millisecond timeout will be as instant as try_recv() therefore makes no sense IMO.

[akumaigorodski]

Factually my original change fixed 100% CPU core issue while retaining all the functionality. My interpretation of that is without idle timeout it was retrying to read immediately on failure, thus taking all available compute resources.

And then staying idle for 1ms seemed like good enough tradeoff between possible added latency and compute use.

Just in case, 100% CPU was not manifesting itself when RPI was connected to SITL simulator via Ethernet/UDP, but started showing up with UART and actual flight controller.

I don't quite like the way it is implemented now, I'd rather remove factory method with hidden logic entirely and let users, for example, provide their own serialport::new (because someone might need different stop bits etc), adding utility methods for every parameter is not scalable.

[onur-ozkan]

I didn't understand what you mean with hidden logic and scalibility to be honest. Can you say more? Scale what exactly? What is hidden ?

[akumaigorodski]

Basically I'm proposing to remove a higher level mavlink::connect with different types of strings and its parsing logic, and let users assemble each invariant from their own parts. If this was in place then I would never have an issue configuring my own serialport::new.

[onur-ozkan]

Just to be sure, what was broken ?

recv() can time out after 1 millisecond when there are no messages available. If someone wants non-blocking, one-shot read they can use try_read instead. A 1 millisecond timeout will be as instant as try_recv() therefore makes no sense IMO.

My claim about recv() should be wrong here. I forgot that we are doing a loop inside recv(). I thought the timeout was applied to whole recv().

So there shouldn't be anything broken really.

[onur-ozkan]

Basically I'm proposing to remove a higher level mavlink::connect with different types of strings and its parsing logic, and let users assemble each invariant from their own parts. If this was in place then I would never have an issue configuring my own serialport::new.

You could use try_recv to avoid all this. Then you can have your own way to receive messages with any specific timeout.

[onur-ozkan]

Changed the approach. The default timeout now remains at 1 ms. I think the 100% CPU usage was caused by having no timeout i.e the lopp was constantly spinning in recv(). With a 1 ms timeout each iteration is blocked by the serial port for up to 1 ms which prevents the busy loop. From that aspect the default 1ms timeout makes sense. But it's still worth keeping it configurable since different use cases may benefit from different timeout values.