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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba0abc6
[FIX] SocketServerProvider.cs: Don't send error code to DataReceived
Viatorus Jan 14, 2021
d601048
[FIX] STM32_UART.cs: Make thread safe
Viatorus Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/Emulator/Main/Utilities/SocketServerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,18 @@ private void ReaderThreadBody(Stream stream)
value = -1;
}

var dataReceived = DataReceived;
if(dataReceived != null)
{
dataReceived(value);
}

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 •
edited
Loading

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!

if(dataReceived != null)
{
dataReceived(value);
}
}
}

Expand Down
55 changes: 39 additions & 16 deletions src/Emulator/Peripherals/Peripherals/UART/STM32_UART.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,28 @@ public void WriteChar(byte value)
this.Log(LogLevel.Warning, "Received a character, but the receiver is not enabled, dropping.");
return;
}
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.

{
receiveFifo.Enqueue(value);
readFifoNotEmpty = true;

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.

{
Update();
}
});
}
}

public override void Reset()
{
base.Reset();
receiveFifo.Clear();
lock (receiveFifo)
{
receiveFifo.Clear();
}
}

public uint BaudRate
Expand Down Expand Up @@ -81,6 +94,7 @@ public Bits StopBits

public GPIO IRQ { get; } = new GPIO();

[field: Transient]
public event Action<byte> CharReceived;

private void DefineRegisters()
Expand All @@ -91,7 +105,10 @@ private void DefineRegisters()
.WithTaggedFlag("NF", 2)
.WithFlag(3, FieldMode.Read, valueProviderCallback: _ => false, name: "ORE") // we assume no receive overruns
.WithTaggedFlag("IDLE", 4)
.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.

}) // as these two flags are WZTC, we cannot just calculate their results
.WithFlag(6, out transmissionComplete, FieldMode.Read | FieldMode.WriteZeroToClear, name: "TC")
.WithFlag(7, FieldMode.Read, valueProviderCallback: _ => true, name: "TXE") // we always assume "transmit data register empty"
.WithTaggedFlag("LBD", 8)
Expand All @@ -102,13 +119,16 @@ private void DefineRegisters()
Register.Data.Define(this, name: "USART_DR")
.WithValueField(0, 9, valueProviderCallback: _ =>
{
uint value = 0;
if(receiveFifo.Count > 0)
byte value = 0;
lock (receiveFifo)
{
value = receiveFifo.Dequeue();
if(receiveFifo.Count > 0)
{
value = receiveFifo.Dequeue();
Update();
}
readFifoNotEmpty = receiveFifo.Count > 0;
}
readFifoNotEmpty.Value = receiveFifo.Count > 0;
Update();
return value;
}, writeCallback: (_, value) =>
{
Expand Down Expand Up @@ -164,11 +184,14 @@ private void DefineRegisters()

private void Update()
{
IRQ.Set(
(receiverNotEmptyInterruptEnabled.Value && readFifoNotEmpty.Value) ||
(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.

It's a debug tool from Rider: https://www.jetbrains.com/help/rider/Debugging_Multithreaded_Applications.html#parallel-stacks

{
IRQ.Set(
(receiverNotEmptyInterruptEnabled.Value && readFifoNotEmpty) ||
(transmitDataRegisterEmptyInterruptEnabled.Value) || // TXE is assumed to be true
(transmissionCompleteInterruptEnabled.Value && transmissionComplete.Value)
);
});
}

private readonly uint frequency;
Expand All @@ -183,7 +206,7 @@ private void Update()
private IFlagRegisterField receiverNotEmptyInterruptEnabled;
private IFlagRegisterField receiverEnabled;
private IFlagRegisterField transmitterEnabled;
private IFlagRegisterField readFifoNotEmpty;
private bool readFifoNotEmpty = false;
private IFlagRegisterField transmissionComplete;
private IValueRegisterField dividerMantissa;
private IValueRegisterField dividerFraction;
Expand Down