InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.65k stars 912 forks source link

Remove use of atomic #1681

Closed Riksu9000 closed 7 months ago

Riksu9000 commented 1 year ago

PineTime and InfiniTime are single threaded.

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 406508B -128B
data 940B 0B
bss 53568B 0B
minacode commented 1 year ago

Do you know how the concurrent tasks from RTOS work? Are they still safe?

Riksu9000 commented 1 year ago

https://freertos.org/implementation/a00004.html It's not possible to execute code concurrently without multiple cores.

minacode commented 1 year ago

That is wrong and your link describes that. A scheduler switches between the tasks, even on one core. It's not happening at the same time, but a switch at an unfortunate time may still cause harm. Therefore my question is if the atomic is needed because there is concurrency going on in InfiniTime.

Riksu9000 commented 1 year ago

That's contradictory. If the tasks are switched, i.e. "not happening at the same time", they're by definition not concurrent. The processor is not able to execute a read and a write operation concurrently. I think if a task switch occurs while copying a large amount of shared data, the other task can potentially access incomplete data, but that's not what this PR is about.

minacode commented 1 year ago

You are confusing parallel with concurrent. Link

The atomic bools in question here are used across tasks (looks like that, haven't checked). Using something that synchronizes well in such a place is a good idea and we should maybe even have more of that.

We should find out what exactly atomic does and if it helps preventing an issue in our case. But neither has InfiniTime only one thread nor are we safe of data races because the processor is not capable of parallelism.

Riksu9000 commented 1 year ago

I wasn't aware of the definitions, but I still can't see the issue. Reading and writing a bool is a single instruction, which is completed before handling the interrupt, so there's no point where a task switch occurring can cause undefined behavior.

minacode commented 1 year ago

With atomics you can read and write the bool without interrupt for example. The question is if that is something we do. I will take a closer look at the code.

Riksu9000 commented 1 year ago

Getting slightly off topic, but I'm curious, can you give me an example where we should use std::atomic on a type that fits in a register? I believe the behavior is well defined and any issues that can occur from accessing a variable from multiple places aren't issues that are handled by atomic.

minacode commented 1 year ago

I am definitely not an expert in this, but from my understanding the power is in having atomic functions that can do multiple operations without interrupt. For example exchange can read a value and then write it and guarantee you that both or nothing happened.

If we use things like atomics we should require an explanation in the code.

Riksu9000 commented 1 year ago

I think I now understand the second issue that atomic can help with, which is what I believe you were concerned with. For example int += 42 in two places can result in only one effective addition, if the value is read but not written before a task switch. In our case I believe we should instead be using FreeRTOS semaphores, which will immediately block and switch the task, and critical sections, which can't switch, because atomic will create a loop and do nothing until the kernel takes over.

minacode commented 1 year ago

Yes, I agree. Concurrency safety is a topic that might be worth a technical discussion and a larger goal for the future. We can also see that issues occur when multiple parts of the software simultaneously use the same hardware. The motor is the best example.