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

Jump to bottom

Fix OSD task timing when using MSP #13388

Merged
merged 1 commit into from
Mar 11, 2024
Merged

Fix OSD task timing when using MSP #13388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 16 additions & 10 deletions src/main/drivers/serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,16 @@ void serialWrite(serialPort_t *instance, uint8_t ch)
}


void serialWriteBuf(serialPort_t *instance, const uint8_t *data, int count)
void serialWriteBufNoFlush(serialPort_t *instance, const uint8_t *data, int count)
{
if (instance->vTable->writeBuf) {
instance->vTable->writeBuf(instance, data, count);
} else {
for (const uint8_t *p = data; count > 0; count--, p++) {

while (!serialTxBytesFree(instance)) {
};
// The transmit buffer is large enough to hold any single message, so only wait once
Copy link
Contributor

Choose a reason for hiding this comment

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

(this will cause system freeze when someone tries to write more. But it may be better that busy-waiting for long time)
Maybe we can change it into nonblocking-write semantics - just write data the fit buffer and return number of bytes written.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked and the transmit buffer never gets close to filling up, so this is fallback behaviour. Rather than dropping characters this would indeed block and the task would be late and if bad enough detected by the CPU load check. I consider this preferable to random MSP dropped packets and resulting strange undefined OSD behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

And it's worth noting that the check is now only being done once which is more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Buffer filling is bad - it will cause busy waiting, which will degrade performance. But if some code tries to write more than transmit buffer size, it will immediately crash FC.

while (serialTxBytesFree(instance) < (uint32_t)count) {
};

for (const uint8_t *p = data; count > 0; count--, p++) {
serialWrite(instance, *p);
}
}
Expand Down Expand Up @@ -107,11 +107,6 @@ void serialSetBaudRateCb(serialPort_t *serialPort, void (*cb)(serialPort_t *cont
}
}

void serialWriteBufShim(void *instance, const uint8_t *data, int count)
{
serialWriteBuf((serialPort_t *)instance, data, count);
}

void serialBeginWrite(serialPort_t *instance)
{
if (instance->vTable->beginWrite)
Expand All @@ -123,3 +118,14 @@ void serialEndWrite(serialPort_t *instance)
if (instance->vTable->endWrite)
instance->vTable->endWrite(instance);
}

void serialWriteBuf(serialPort_t *instance, const uint8_t *data, int count)
ledvinap marked this conversation as resolved.
Show resolved Hide resolved
{
serialBeginWrite(instance);
serialWriteBufNoFlush(instance, data, count);
serialEndWrite(instance);
}
void serialWriteBufShim(void *instance, const uint8_t *data, int count)
Copy link
Contributor

Choose a reason for hiding this comment

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

(technically, this function belongs to buf_write.[ch])

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite sure what badness we're forcing on the user here.

It's common code irrespective of UART implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but this interface is necessary/used only for bufWrite functions. So it is not serial-related, but buf_write related function.

{
serialWriteBuf((serialPort_t *)instance, data, count);
}
1 change: 1 addition & 0 deletions src/main/drivers/serial.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ void serialWrite(serialPort_t *instance, uint8_t ch);
uint32_t serialRxBytesWaiting(const serialPort_t *instance);
uint32_t serialTxBytesFree(const serialPort_t *instance);
void serialWriteBuf(serialPort_t *instance, const uint8_t *data, int count);
void serialWriteBufNoFlush(serialPort_t *instance, const uint8_t *data, int count);
uint8_t serialRead(serialPort_t *instance);
void serialSetBaudRate(serialPort_t *instance, uint32_t baudRate);
void serialSetMode(serialPort_t *instance, portMode_e mode);
Expand Down
73 changes: 70 additions & 3 deletions src/main/drivers/serial_uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,15 @@

#include <stdbool.h>
#include <stdint.h>
#include <string.h>

#include "platform.h"

#ifdef USE_UART

#include "build/build_config.h"

#include <common/maths.h>
#include "common/utils.h"

#include "drivers/dma.h"
Expand Down Expand Up @@ -297,6 +299,71 @@ static void uartWrite(serialPort_t *instance, uint8_t ch)
}
}

static void uartBeginWrite(serialPort_t *instance)
{
uartPort_t *uartPort = (uartPort_t *)instance;

// Check if the TX line is being pulled low by an unpowered peripheral
if (uartPort->checkUsartTxOutput) {
uartPort->checkUsartTxOutput(uartPort);
}
}

static void uartWriteBuf(serialPort_t *instance, const void *data, int count)
{
uartPort_t *uartPort = (uartPort_t *)instance;
uartDevice_t *uart = container_of(uartPort, uartDevice_t, port);
const uint8_t *bytePtr = (const uint8_t*)data;

// Test if checkUsartTxOutput() detected TX line being pulled low by an unpowered peripheral
if (uart->txPinState == TX_PIN_MONITOR) {
// TX line is being pulled low, so don't transmit
return;
}

while (count > 0) {
// Calculate the available space to the end of the buffer
const int spaceToEnd = uartPort->port.txBufferSize - uartPort->port.txBufferHead;
// Determine the amount to copy in this iteration
const int chunkSize = MIN(spaceToEnd, count);
// Copy the chunk
memcpy((void *)&uartPort->port.txBuffer[uartPort->port.txBufferHead], bytePtr, chunkSize);
// Advance source pointer
bytePtr += chunkSize;
// Advance head, wrapping if necessary
uartPort->port.txBufferHead = (uartPort->port.txBufferHead + chunkSize) % uartPort->port.txBufferSize;
// Decrease remaining count
count -= chunkSize;
}
}

static void uartEndWrite(serialPort_t *instance)
{
uartPort_t *uartPort = (uartPort_t *)instance;
uartDevice_t *uart = container_of(uartPort, uartDevice_t, port);

// Check if the TX line is being pulled low by an unpowered peripheral
SteveCEvans marked this conversation as resolved.
Show resolved Hide resolved
if (uart->txPinState == TX_PIN_MONITOR) {
// TX line is being pulled low, so don't transmit
return;
}

#ifdef USE_DMA
if (uartPort->txDMAResource) {
uartTryStartTxDMA(uartPort);
} else
#endif
{
#if defined(USE_HAL_DRIVER)
__HAL_UART_ENABLE_IT(&uartPort->Handle, UART_IT_TXE);
#elif defined(USE_ATBSP_DRIVER)
usart_interrupt_enable(uartPort->USARTx, USART_TDBE_INT, TRUE);
#else
USART_ITConfig(uartPort->USARTx, USART_IT_TXE, ENABLE);
#endif
}
}

const struct serialPortVTable uartVTable[] = {
{
.serialWrite = uartWrite,
Expand All @@ -308,9 +375,9 @@ const struct serialPortVTable uartVTable[] = {
.setMode = uartSetMode,
.setCtrlLineStateCb = NULL,
.setBaudRateCb = NULL,
.writeBuf = NULL,
.beginWrite = NULL,
.endWrite = NULL,
.writeBuf = uartWriteBuf,
.beginWrite = uartBeginWrite,
.endWrite = uartEndWrite,
}
};

Expand Down
6 changes: 3 additions & 3 deletions src/main/msp/msp_serial.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,9 @@ static int mspSerialSendFrame(mspPort_t *msp, const uint8_t * hdr, int hdrLen, c

// Transmit frame
serialBeginWrite(msp->port);
serialWriteBuf(msp->port, hdr, hdrLen);
serialWriteBuf(msp->port, data, dataLen);
serialWriteBuf(msp->port, crc, crcLen);
serialWriteBufNoFlush(msp->port, hdr, hdrLen);
serialWriteBufNoFlush(msp->port, data, dataLen);
serialWriteBufNoFlush(msp->port, crc, crcLen);
serialEndWrite(msp->port);

return totalFrameLength;
Expand Down
131 changes: 40 additions & 91 deletions src/main/osd/osd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,13 +206,7 @@ const osd_stats_e osdStatsDisplayOrder[OSD_STAT_COUNT] = {
#define OSD_GROUP_COUNT OSD_ITEM_COUNT
// Aim to render a group of elements within a target time
#define OSD_ELEMENT_RENDER_TARGET 30
// Allow a margin by which a group render can exceed that of the sum of the elements before declaring insane
// This will most likely be violated by a USB interrupt whilst using the CLI
#if defined(STM32F411xE)
#define OSD_ELEMENT_RENDER_GROUP_MARGIN 7
#else
#define OSD_ELEMENT_RENDER_GROUP_MARGIN 2
#endif

#define OSD_TASK_MARGIN 1
// Decay the estimated max task duration by 1/(1 << OSD_EXEC_TIME_SHIFT) on every invocation
#define OSD_EXEC_TIME_SHIFT 8
Expand Down Expand Up @@ -1339,8 +1333,8 @@ typedef enum {
OSD_STATE_PROCESS_STATS2,
OSD_STATE_PROCESS_STATS3,
OSD_STATE_UPDATE_ALARMS,
OSD_STATE_REFRESH_PREARM,
OSD_STATE_UPDATE_CANVAS,
OSD_STATE_GROUP_ELEMENTS,
OSD_STATE_UPDATE_ELEMENTS,
OSD_STATE_UPDATE_HEARTBEAT,
OSD_STATE_COMMIT,
Expand Down Expand Up @@ -1379,13 +1373,8 @@ bool osdUpdateCheck(timeUs_t currentTimeUs, timeDelta_t currentDeltaTimeUs)
void osdUpdate(timeUs_t currentTimeUs)
{
static uint16_t osdStateDurationFractionUs[OSD_STATE_COUNT] = { 0 };
static uint32_t osdElementDurationUs[OSD_ITEM_COUNT] = { 0 };
static uint8_t osdElementGroupMemberships[OSD_ITEM_COUNT];
static uint16_t osdElementGroupTargetFractionUs[OSD_GROUP_COUNT] = { 0 };
static uint16_t osdElementGroupDurationFractionUs[OSD_GROUP_COUNT] = { 0 };
static uint8_t osdElementGroup;
static uint32_t osdElementDurationFractionUs[OSD_ITEM_COUNT] = { 0 };
static bool firstPass = true;
uint8_t osdCurrentElementGroup = 0;
timeUs_t executeTimeUs;
osdState_e osdCurrentState = osdState;

Expand Down Expand Up @@ -1473,8 +1462,29 @@ void osdUpdate(timeUs_t currentTimeUs)
if (resumeRefreshAt) {
osdState = OSD_STATE_TRANSFER;
} else {
#ifdef USE_SPEC_PREARM_SCREEN
osdState = OSD_STATE_REFRESH_PREARM;
#else
osdState = OSD_STATE_UPDATE_CANVAS;
#endif
}
break;

case OSD_STATE_REFRESH_PREARM:
{
#ifdef USE_SPEC_PREARM_SCREEN
if (!ARMING_FLAG(ARMED) && osdConfig()->osd_show_spec_prearm) {
if (osdDrawSpec(osdDisplayPort)) {
// Rendering is complete
osdState = OSD_STATE_COMMIT;
}
} else
#endif // USE_SPEC_PREARM_SCREEN
{
osdState = OSD_STATE_UPDATE_CANVAS;
}
}

break;

case OSD_STATE_UPDATE_CANVAS:
Expand Down Expand Up @@ -1508,90 +1518,42 @@ void osdUpdate(timeUs_t currentTimeUs)

osdSyncBlink();

osdState = OSD_STATE_GROUP_ELEMENTS;
osdState = OSD_STATE_UPDATE_ELEMENTS;

break;

case OSD_STATE_GROUP_ELEMENTS:
{
uint8_t elementGroup;
uint8_t activeElements = osdGetActiveElementCount();

// Reset groupings
for (elementGroup = 0; elementGroup < OSD_GROUP_COUNT; elementGroup++) {
if (osdElementGroupDurationFractionUs[elementGroup] > (OSD_ELEMENT_RENDER_TARGET << OSD_EXEC_TIME_SHIFT)) {
osdElementGroupDurationFractionUs[elementGroup] = 0;
}
osdElementGroupTargetFractionUs[elementGroup] = 0;
}

elementGroup = 0;

// Based on the current element rendering, group to execute in approx 40us
for (uint8_t curElement = 0; curElement < activeElements; curElement++) {
if ((osdElementGroupTargetFractionUs[elementGroup] == 0) ||
(osdElementGroupTargetFractionUs[elementGroup] + (osdElementDurationUs[curElement]) <= (OSD_ELEMENT_RENDER_TARGET << OSD_EXEC_TIME_SHIFT)) ||
(elementGroup == (OSD_GROUP_COUNT - 1))) {
osdElementGroupTargetFractionUs[elementGroup] += osdElementDurationUs[curElement];
// If group membership changes, reset the stats for the group
if (osdElementGroupMemberships[curElement] != elementGroup) {
osdElementGroupDurationFractionUs[elementGroup] = osdElementGroupTargetFractionUs[elementGroup] + (OSD_ELEMENT_RENDER_GROUP_MARGIN << OSD_EXEC_TIME_SHIFT);
}
osdElementGroupMemberships[curElement] = elementGroup;
} else {
elementGroup++;
// Try again for this element
curElement--;
}
}

// Start with group 0
osdElementGroup = 0;

if (activeElements > 0) {
osdState = OSD_STATE_UPDATE_ELEMENTS;
} else {
osdState = OSD_STATE_COMMIT;
}
}
break;

case OSD_STATE_UPDATE_ELEMENTS:
{
osdCurrentElementGroup = osdElementGroup;
bool moreElements = true;

do {
timeUs_t startElementTime = micros();
for (int rendered = 0; moreElements; rendered++) {
uint8_t osdCurrentElement = osdGetActiveElement();

// This element should be rendered in the next group
if (osdElementGroupMemberships[osdCurrentElement] != osdElementGroup) {
osdElementGroup++;
timeUs_t startElementTime = micros();

timeUs_t anticipatedEndUs = startElementTime + (osdElementDurationFractionUs[osdCurrentElement] >> OSD_EXEC_TIME_SHIFT);

if ((rendered > 0) && cmpTimeUs(anticipatedEndUs, currentTimeUs) > OSD_ELEMENT_RENDER_TARGET) {
// There isn't time to render the next element
break;
}

moreElements = osdDrawNextActiveElement(osdDisplayPort, currentTimeUs);

executeTimeUs = micros() - startElementTime;

if (executeTimeUs > (osdElementDurationUs[osdCurrentElement] >> OSD_EXEC_TIME_SHIFT)) {
osdElementDurationUs[osdCurrentElement] = executeTimeUs << OSD_EXEC_TIME_SHIFT;
} else if (osdElementDurationUs[osdCurrentElement] > 0) {
if (executeTimeUs > (osdElementDurationFractionUs[osdCurrentElement] >> OSD_EXEC_TIME_SHIFT)) {
osdElementDurationFractionUs[osdCurrentElement] = executeTimeUs << OSD_EXEC_TIME_SHIFT;
} else if (osdElementDurationFractionUs[osdCurrentElement] > 0) {
// Slowly decay the max time
osdElementDurationUs[osdCurrentElement]--;
osdElementDurationFractionUs[osdCurrentElement]--;
}
} while (moreElements);
};

if (moreElements) {
// There are more elements to draw
break;
}
#ifdef USE_SPEC_PREARM_SCREEN
osdDrawSpec(osdDisplayPort);
#endif // USE_SPEC_PREARM_SCREEN

osdElementGroup = 0;

osdState = OSD_STATE_COMMIT;
}
Expand Down Expand Up @@ -1636,15 +1598,6 @@ void osdUpdate(timeUs_t currentTimeUs)
// On the first pass no element groups will have been formed, so all elements will have been
// rendered which is unrepresentative, so ignore
if (!firstPass) {
if (osdCurrentState == OSD_STATE_UPDATE_ELEMENTS) {
if (executeTimeUs > (osdElementGroupDurationFractionUs[osdCurrentElementGroup] >> OSD_EXEC_TIME_SHIFT)) {
osdElementGroupDurationFractionUs[osdCurrentElementGroup] = executeTimeUs << OSD_EXEC_TIME_SHIFT;
} else if (osdElementGroupDurationFractionUs[osdCurrentElementGroup] > 0) {
// Slowly decay the max time
osdElementGroupDurationFractionUs[osdCurrentElementGroup]--;
}
}

if (executeTimeUs > (osdStateDurationFractionUs[osdCurrentState] >> OSD_EXEC_TIME_SHIFT)) {
osdStateDurationFractionUs[osdCurrentState] = executeTimeUs << OSD_EXEC_TIME_SHIFT;
} else if (osdStateDurationFractionUs[osdCurrentState] > 0) {
Expand All @@ -1654,14 +1607,10 @@ void osdUpdate(timeUs_t currentTimeUs)
}
}

if (osdState == OSD_STATE_UPDATE_ELEMENTS) {
schedulerSetNextStateTime((osdElementGroupDurationFractionUs[osdElementGroup] >> OSD_EXEC_TIME_SHIFT) + OSD_ELEMENT_RENDER_GROUP_MARGIN);
if (osdState == OSD_STATE_IDLE) {
schedulerSetNextStateTime((osdStateDurationFractionUs[OSD_STATE_CHECK] >> OSD_EXEC_TIME_SHIFT) + OSD_TASK_MARGIN);
} else {
if (osdState == OSD_STATE_IDLE) {
schedulerSetNextStateTime((osdStateDurationFractionUs[OSD_STATE_CHECK] >> OSD_EXEC_TIME_SHIFT) + OSD_TASK_MARGIN);
} else {
schedulerSetNextStateTime((osdStateDurationFractionUs[osdState] >> OSD_EXEC_TIME_SHIFT) + OSD_TASK_MARGIN);
}
schedulerSetNextStateTime((osdStateDurationFractionUs[osdState] >> OSD_EXEC_TIME_SHIFT) + OSD_TASK_MARGIN);
}
}

Expand Down