Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#169] STM32: make UART thread safe #27

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Viatorus
Copy link

@Viatorus Viatorus commented Jan 14, 2021

@CLAassistant
Copy link

CLAassistant commented Jan 14, 2021

CLA assistant check
All committers have signed the CLA.

@Viatorus Viatorus changed the title make STM32 uart thread safe [#169] make STM32 uart thread safe Jan 14, 2021
@Viatorus Viatorus changed the title [#169] make STM32 uart thread safe [#169] STM32: make UART thread safe Jan 14, 2021
if(value == -1)
{
Logger.LogAs(this, LogLevel.Debug, "Client disconnected, stream closed.");
writerCancellationToken.Cancel();
break;
}

var dataReceived = DataReceived;
Copy link
Author

@Viatorus Viatorus Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:
I changed the order, otherwise the value -1 would be used as dataReceived!

receiveFifo.Enqueue(value);
readFifoNotEmpty.Value = true;
Update();
lock (receiveFifo)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: The queue was used in two threads but never locked. This led to crashes.

(transmitDataRegisterEmptyInterruptEnabled.Value) || // TXE is assumed to be true
(transmissionCompleteInterruptEnabled.Value && transmissionComplete.Value)
);
machine.ClockSource.ExecuteInLock(() =>
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

Here is a possible dead lock. I don't like my fix but it worked for me.

machine thread is inside BaseClockSource (locked) and tries to lock the NVIC.

_Reader_Thread updates the NVIC (locked) and then tries to lock the BaseClockSource which is locked by machine thread.

Screenshot_20210114_142013

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Viatorus Sorry, How do you generate this figure? Could you please tell me the name of the tool?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.WithFlag(5, out readFifoNotEmpty, FieldMode.Read | FieldMode.WriteZeroToClear, name: "RXNE") // as these two flags are WZTC, we cannot just calculate their results
.WithFlag(5, FieldMode.Read | FieldMode.WriteZeroToClear, name: "RXNE", valueProviderCallback: _ =>
{
return readFifoNotEmpty;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

This workaround I don't understand my self but otherwise, it is not working.

if readFifoNotEmpty is a IFlagRegisterField, it happens that the machine does not see the change and the application stucks.

If we put inside the callback function, this is not happening anymore.


machine.ClockSource.ExecuteInLock(() =>
{
if (receiverNotEmptyInterruptEnabled.Value)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

So there is the case, that the machine is not yet done with the last interrupt handling:

receiverNotEmptyInterruptEnabled is not yet set to true but transmissionCompleteInterruptEnabled and transmissionComplete are so the Update() method would wrongly set IRQ to true again.

If this happens, the IRQ hangs somehow.

Overall I am not a fan of updating the IRQ from a different thread than the machine thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants