AsyncCanDriver is not Sync

[leonardodanelutti]

AsyncCanDriver is not Sync because TaskHandle_t is not Sync. AsyncUartDriver has also a field with type TaskHandle_t but Sync is implemented for it.

[ivmarkov]

Why do you need it to be sync in the first place?

[ivmarkov]

I mean, a lot of the methods currently take & mut self so it is not just the task handle.

[leonardodanelutti]

@ivmarkov I was doing some testing with LocalExecutor and Executor from the edge_executor crate with an esp32-s3. The best solution is probably to have one LocalExecutor per core, but that's not the point. As far as I know the twai driver is thread safe and if this is the case I don't understand why Sync should not be implemented. There seems to be a lot of inconsistency about the drivers being Sync and/or Send throughout the crate. But surely I am wrong and there is an explanation for each one of them. In this case I would suggest adding comments explaining the reason for these choices.

[ivmarkov]

@ivmarkov I was doing some testing with LocalExecutor and Executor from the edge_executor crate with an esp32-s3. The best solution is probably to have one LocalExecutor per core, but that's not the point. As far as I know the twai driver is thread safe and if this is the case I don't understand why Sync should not be implemented. There seems to be a lot of inconsistency about the drivers being Sync and/or Send throughout the crate. But surely I am wrong and there is an explanation for each one of them. In this case I would suggest adding comments explaining the reason for these choices.

Look at the reply I gave you for the issue where one of the SPI drivers is not Send. I hope it sums up the status quo pretty nicely. But maybe again:

• ALL drivers should be Send. If you find a driver which is not Send, this is a bug, needs to be fixed.
• Some drivers might be Sync but so far we have never chased formally exposing the thread safety of the ESP IDF drivers/services, as it is often very poorly documented. Sometimes it is mentioned with 5 words like "this API is thread safe", sometimes we don't even have that. As an example, right now I'm implementing the GATT/BLE bindings for ESP IDF's Bluedroid and there is. no. single. statement. w.r.t. the thread safety of this complex API. Just one very misleading comment in the Esp32 forums. And unlike for the "regular" drivers' case, here I really really do need to know if I can make / expose a thread safe variant of the API, due to how multiple higher layer BT drivers are stacked on top of the base BtDriver and work concurrently with each other. So I had to examine the C code in details so as to realize, that while Bluedriod itself is unlikely to be thread-safe, the ESP IDF wrappers isolate it to a dedicated task and the whole ESP IDF API works by posting to this task. So everything except setting the callbacks for event notifications is actually thread safe, it seems.

So you understand, in this situation, the safer bet is always to expose something as not thread-safe, rather than as thread-safe when it turns out later it is not (and you have to make backwards-incompatible changes; going the other way, from non-thread safe to thread safe is backwards compatible).

When I was mentioning that folks often stumble on the need to have something Sync when they (most likely) wrongly try to use work-stealing executor (like edge_executor::Executor) I meant exactly that - having the requirement your Futures to be Send (which means the driver should be Sync and all &mut self methods should become &self) is a heavy requirement, yet for embedded - at least for now - to me it always had a questionable value. Perhaps you can share your thoughts / view on the subject?

With that said... if you or other folks feel Send futures are important, and actually help me - driver by driver - with PRs - to expose a Sync variant of those drivers which are thread safe in ESP IDF - I'm all for that! But I really do need help here, as most of this work is on my shoulders only, few people understand it, and I often end just with issues by folks complaining and with no or very low quality PRs as folks simply do not understand these things, as they are above a certain complexity margin.

[leonardodanelutti]

Thanks for the reply, yes you are right if we don't have confidence that a driver is thread-safe we shouldn't expose it as such. What bothered me is the fact that some drivers are Sync and I couldn't find an explanation why. For example LedcDriver and the TxRmtDriver are Sync even though they have methods that take &mut self (and now that I checked they also expose methods that are not thread-safe). For the CanDriver since there is no information I thought all the methods were threrad-safe but, as you rightly said, we would have to analyze the C code to be sure.
I don't think it is a priority to have send futures but I think it is ok to have them if it is possible to implement them, the choice of the Executor to use should be of the user not imposed by the library.
Unfortunately I don't think I fall among those few people and I don't want to waste your time with more low quality PRs, keep up the good work!

[ivmarkov]

Thanks for the reply, yes you are right if we don't have confidence that a driver is thread-safe we shouldn't expose it as such. What bothered me is the fact that some drivers are Sync and I couldn't find an explanation why. For example LedcDriver and the TxRmtDriver are Sync even though they have methods that take &mut self (and now that I checked they also expose methods that are not thread-safe).

These drivers were contributions by other folks, and marking these as Sync was probably an omission on the side of the contributors, and of course also my omission as I did not catch this.

For the CanDriver since there is no information I thought all the methods were threrad-safe but, as you rightly said, we would have to analyze the C code to be sure.

ESP IDF generally advertises itself as "most APIs are thread-safe unless stated otherwise". How much that statement can be trusted without carefully examining the C source code... I think we agree we should be careful here. Yet I think I'm warming up to the idea of - over time - exposing as thread safe APIs these which we have checked in C to be really thread safe.

I don't think it is a priority to have send futures but I think it is ok to have them if it is possible to implement them, the choice of the Executor to use should be of the user not imposed by the library.

Well it is to some extent also a library decision. As it is the library which decides whether to expose Send futures or not, and as per above, exposing Send futures might mean a lot of work for diminishing results, so that's why - at least for me - it had been second priority so far. Also let's not forget that this is embedded. Very little RAM. Every additional thread means extra stack you need to care about as in sizing. I'm therefore in the camp that believes - unless you have really good reasons to spin extra threads (as in - let's say - the need to preemptively execute a very high priority task), app threads should be kept to a minimum. Ideally, just one thread which executes all your tasks. But if you do this, then you don't need your futures to be Send as you don't schedule them across multiple threads in the first place. Not to mention that you can have multiple local executors whose tasks communicate across thread boundaries using synchronization primitives which are thread safe.

Unfortunately I don't think I fall among those few people and I don't want to waste your time with more low quality PRs, keep up the good work!

Don't be offended by the "low quality PRs", as I didn't meant to offend you, nor anybody else! It is just the reality a bit is that we have these, and to get these through, there is a need to do a lot of hand holding (and persistence on the contributor side!).

I think the actual problem is, there are very few active contributors willing to dedicate a sizeable amount of their time to learn it all on these crates. :)

[leonardodanelutti]

Well it is to some extent also a library decision. As it is the library which decides whether to expose Send futures or not, and as per above, exposing Send futures might mean a lot of work for diminishing results, so that's why - at least for me - it had been second priority so far.

Yes I agree with you. I was referring to all those drivers that are thread safe, if we don't need to introduce some primitive for synchronization I don't think we should omit the Sync implementation. If, on the other hand, they are not thread-safe I think the opposite is true.

Also let's not forget that this is embedded. Very little RAM. Every additional thread means extra stack you need to care about as in sizing. I'm therefore in the camp that believes - unless you have really good reasons to spin extra threads (as in - let's say - the need to preemptively execute a very high priority task), app threads should be kept to a minimum. Ideally, just one thread which executes all your tasks. But if you do this, then you don't need your futures to be Send as you don't schedule them across multiple threads in the first place. Not to mention that you can have multiple local executors whose tasks communicate across thread boundaries using synchronization primitives which are thread safe.

Yes most of the time I don't think send futures are necessary but on some occasions they might be useful. With two cores, as you said, you can use two local executors but it is not always easy to figure out how best to distribute tasks between the two. Maybe it may even be impossible if the amount of work of each task varies a lot over time.

But all this maybe is a bit off-topic for this issue :)