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

Buffer Indexes are not atomic when uint16_t(LARGE_BUFFER) #18

Open
jmj073 opened this issue Oct 12, 2022 · 0 comments
Open

Buffer Indexes are not atomic when uint16_t(LARGE_BUFFER) #18

jmj073 opened this issue Oct 12, 2022 · 0 comments

Comments

@jmj073
Copy link

jmj073 commented Oct 12, 2022

I am using a translator, so please understand. Thank you sir.

change the getc() code location

The code below is only applied to uart1, so it seems like it should be applied to other codes as well.

/* uart1_getc */
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    if (UART1_RxHead == UART1_RxTail) {
        return UART_NO_DATA;   /* no data available */
    }
    /* calculate / store buffer index */
    tmptail = (UART1_RxTail + 1) & UART_RX1_BUFFER_MASK;
    UART1_RxTail = tmptail;
}
/* uart0_getc */
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
    if (UART_RxHead == UART_RxTail) {
        return UART_NO_DATA;   /* no data available */
    }
}
/* calculate / store buffer index */
tmptail = (UART_RxTail + 1) & UART_RX0_BUFFER_MASK;
UART_RxTail = tmptail; // <- This is not atomic when LARGE_BUFFER.

I think it would be better to do something like below. It was also applied to peek.

/* uart1_getc */
#ifdef USART1_LARGE_BUFFER
ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
#endif
{
    if (UART1_RxHead == UART1_RxTail) {
        return UART_NO_DATA;   /* no data available */
    }
    /* calculate / store buffer index */
    tmptail = (UART1_RxTail + 1) & UART_RX1_BUFFER_MASK;
    UART1_RxTail = tmptail;
}

putc atomic

When LARGE_BUFFER, UART1_TxHead is uint16_t. So the code below is not atomic.

UART1_TxHead = tmphead;

I think it would be good to change it atomically as follows.

#ifdef USART1_LARGE_BUFFER
ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
#endif
{
	UART1_TxHead = tmphead;
}

There are a lot of duplicates, so I think it would be better to do something like this:

#ifdef USART0_LARGE_BUFFER
	#define UART0_ATOMIC_BLOCK ATOMIC_BLOCK(ATOMIC_RESTORESTATE)
#else
	#define UART0_ATOMIC_BLOCK
#endif

If you agree with this, I'll put in a pull request.

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

No branches or pull requests

1 participant