You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/27 02:40:01 UTC
[GitHub] [incubator-nuttx] CV-Bowen opened a new pull request, #6334: arm/tlsr82: ble performance optimize and problems solve.
CV-Bowen opened a new pull request, #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334
## Summary
RF and system timer interrupt are used for ble.
tlsr82_flash.c:
1. BLE will loss packets during flash operation beacause the interrupt
is disabled and the operation take too long (especially erasing,
about 100ms), so allow RF and system timer interrupt during flash
operation;
2. Add sched_lock()/sched_unlock() to avoid the task switch in ble and
system timer interrupt;
flash_boot_ble.ld:
3. Because of 1, the code executes in RF and system timer interrupt
must be in ram to avoid bus error. The sem_post() will be called and
const variable g_tasklisttable will be accessed in RF and system
timer interrupt handler;
4. To improve the performance, copy some frequently called function to
ram as well, such as: sem_take(), sched_lock(), sched_unlock(),
some lib functions, some zephyr ble functions and some tinycrypt
functions;
5. The RF and system timer interrupt handler will call some libgcc
functions, so copy all the libgcc functions to ram exclude _divdi3.o,
_udivdi3.o and _umoddi3.o;
tlsr82_serial.c
6. Make up_putc() be thread safe;
tc32_doirq.c
7. Increase the RF and system timer interrupt response priority;
## Impact
tlsr82 ble
## Testing
local test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883376945
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
OK
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883324876
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
Transmit one char with uart cost about 86us (115200 baudrate, 1bit start, 1bit stop, 8bit data). I hope the interrupt disabled time is small, so the interrupt can respond with short delay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883506524
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
`arch/arm/src/tms570/tms570_lowputc.c` has another example on how to deal with that.
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
Review Comment:
Do we need to add `#ifdef HAVE_SERIAL_CONSOLE` wrapper?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883497830
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
can we switch all the places to use `__ramfunc__`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883768508
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
Review Comment:
I posted two separate comments. One related to ifdef wrapper that I think should be applied anyway and the second one related to MT safety. IMO approach with critical section is better because same putc can be used in the ISR as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883755331
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
Review Comment:
This is unrelated to the SERIAL_CONSOLE, even do not enable HAVE_SERIAL_CONSOLE , the problem is still existed, because send byte operation is not atomic (see function `uart_send_byte()`). Maybe a alternative solution is add `enter/leave_critical_section()` in function `uart_send_byte()`, which is similar to the solution show in `arch/arm/src/tms570/tms570_lowputc.c`. What do you think?
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
All the codes have been fully tested, i do not recommend to modify this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883774466
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
Review Comment:
Ack, i will do this later today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883765927
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
Yes. Sure. This is a note for the further development. There is already section in RAM for functions, so maybe the existing section can be renamed to match common approach
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r884515191
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -612,14 +612,20 @@ static inline uint8_t uart_read_byte(int uart_num)
static inline void uart_send_byte(uint8_t byte)
{
+ irqstate_t flags;
+
while (UART_GET_TX_BUF_CNT() > 7);
+ flags = enter_critical_section();
+
UART_BUF(uart_txindex) = byte;
Review Comment:
is it reasonable to check `UART_GET_TX_BUF_CNT() <= 7` after entering critical section and jump to `while (UART_GET_TX_BUF_CNT() > 7);` in case if it is false? I mean trying to handle the case when preemption happens right before `flags = enter_critical_section();` and other writer puts a char.
Again referring to `arch/arm/src/tms570/tms570_lowputc.c`:
```
void arm_lowputc(char ch)
{
#ifdef HAVE_SERIAL_CONSOLE
irqstate_t flags;
for (; ; )
{
/* Wait for the transmitter to be available */
while ((getreg32(TMS570_CONSOLE_BASE + TMS570_SCI_FLR_OFFSET) &
SCI_FLR_TXRDY) == 0);
/* Disable interrupts so that the test and the transmission are
* atomic.
*/
flags = enter_critical_section();
if ((getreg32(TMS570_CONSOLE_BASE + TMS570_SCI_FLR_OFFSET) &
SCI_FLR_TXRDY) != 0)
{
/* Send the character */
putreg32((uint32_t)ch, TMS570_CONSOLE_BASE + TMS570_SCI_TD_OFFSET);
leave_critical_section(flags);
return;
}
leave_critical_section(flags);
}
#endif
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883772825
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
Actually, some external codes provided by telink also used section `.ram_code`, so this can not be easily modified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883765927
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
Yes. Sure. This is a note for the further development. There is already section in RAM for functions, so maybe the existing section can be renamed to match common approach.
So `.ram_code` section can be renamed in linker scripts as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r884531133
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -612,14 +612,20 @@ static inline uint8_t uart_read_byte(int uart_num)
static inline void uart_send_byte(uint8_t byte)
{
+ irqstate_t flags;
+
while (UART_GET_TX_BUF_CNT() > 7);
+ flags = enter_critical_section();
+
UART_BUF(uart_txindex) = byte;
Review Comment:
Ack and Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883267009
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
why not use spinlock_irq_save or enter_critical_section
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r884374966
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
Done
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r884515191
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -612,14 +612,20 @@ static inline uint8_t uart_read_byte(int uart_num)
static inline void uart_send_byte(uint8_t byte)
{
+ irqstate_t flags;
+
while (UART_GET_TX_BUF_CNT() > 7);
+ flags = enter_critical_section();
+
UART_BUF(uart_txindex) = byte;
Review Comment:
is it reasonable to check `UART_GET_TX_BUF_CNT() <= 7` after entering critical section and jump to `while (UART_GET_TX_BUF_CNT() > 7);` in case if it is false? I mean trying to handle the case when preemption happens right before `flags = enter_critical_section();` and other writer puts a char.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] CV-Bowen commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
CV-Bowen commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883329974
##########
arch/arm/src/tlsr82/tlsr82_serial.c:
##########
@@ -1558,6 +1558,10 @@ static inline int tlsr82_uart_lowputc(int ch)
#ifndef CONFIG_TLSR82_SPI_SYSLOG
int up_putc(int ch)
{
+ /* To make the syslog be thread safe, add sched_lock()/sched_unlock() */
+
+ sched_lock();
Review Comment:
> Transmit one char with uart cost about 86us (115200 baudrate, 1bit start, 1bit stop, 8bit data). I hope the interrupt disabled time is small, so the interrupt can respond with short delay.
ADD: `tlsr82_uart_lowputc()` is very fast when the hardware uart tx fifo is not full, but the fifo size is only 8, so in many cases, the fifo is full, and `tlsr82_uart_lowputc()` must wait about 86us (depends on the baudrate) for the free space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6334: arm/tlsr82: ble performance optimize and problems solve.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6334:
URL: https://github.com/apache/incubator-nuttx/pull/6334#discussion_r883777041
##########
arch/arm/src/tlsr82/tc32/tc32_doirq.c:
##########
@@ -102,7 +111,7 @@ static int locate_code(".ram_code") tc32_getirq(void)
*
****************************************************************************/
-void arm_ack_irq(int irq)
+void locate_code(".ram_code") arm_ack_irq(int irq)
Review Comment:
I see. Then it makes sense to keep it as is
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org