wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 611 forks source link

Document thread safety guarantees of HAL SerialPort initialization #2035

Open jkuszmaul opened 5 years ago

jkuszmaul commented 5 years ago

I'm not sure anyone is actually using serial ports from multiple threads, but given that most things in the HAL seem to be written in a way that tries to be thread-safe, I'm assuming that the calls to, e.g., HAL_InitializeSerialPort and HAL_CloseSerial and all the other serial calls defined in hal/src/main/native/athena/SerialPort.cpp are intended to be safe to call from multiple threads.

Currently, after any of the calls to serialPortHandles->Get, there is no longer a mutex protecting access to the SerialPort.

ThadHouse commented 5 years ago

We actually don't guarantee thread safety in the Hal between different objects. In things like DIO, it's required because different DIO objects share the same underlying resource, but it's not something we ensure, and is just an implementation detail. I think we have it documented somewhere, if not we need to add that to the docs.

jkuszmaul commented 5 years ago

Alright, just seemed out of place since there are locks protecting some of the initialization, but not all of it. There does not appear to be any documentation on this in the header file, which is where I'd default to looking.

ThadHouse commented 5 years ago

Yeah. I'll add some docs. But in generate, initialization is thread safe (you'll only get 1 valid handle created though), but anything taking a handle is not thread safe.

jkuszmaul commented 5 years ago

Sounds good. And I guess sense the Allocate will fail if anyone else tries to use it, that should generally prevent issues on initialization.