Skip to content

sys: make uart_stdio RX optional#7974

Closed
kaspar030 wants to merge 1 commit intoRIOT-OS:masterfrom
kaspar030:make_uart_stdio_rx_optional
Closed

sys: make uart_stdio RX optional#7974
kaspar030 wants to merge 1 commit intoRIOT-OS:masterfrom
kaspar030:make_uart_stdio_rx_optional

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

Previously, uart_stdio always set up receiving data from serial.

This led to blocking powermodes (see #7947), and also used memory for the incoming buffer, even if unused.

This PR makes the RX side of uart_stdio available as pseudomodule (uart_stdio_rx).

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pm Area: (Low) power management labels Nov 9, 2017
@kaspar030 kaspar030 added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Nov 9, 2017
@roberthartung
Copy link
Copy Markdown
Member

Taking it futher: How would I disable the uart completely? Because uart is selected automatically?

Copy link
Copy Markdown
Member

@roberthartung roberthartung left a comment

Choose a reason for hiding this comment

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

Looks good, just some questions / comments.

ifneq (,$(filter uart_stdio,$(USEMODULE)))
ifneq (,$(filter uart_stdio_rx,$(USEMODULE)))
USEMODULE += isrpipe
USEMODULE += uart_stdio
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be confusing why there is uart_stdio and uart_stdio_rx - is renaming uart_stdio to uart_stdio_tx useful/helpful/required? Or should we add uart_stdio_tx as a pseudo module just to make it clear but also have the opportunity to have both?

USEMODULE += uart_stdio
endif

ifneq (,$(filter uart_stdio,$(USEMODULE)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we extend this to be USEMODULE += uart_stdio_tx and USEMODULE += uart_stdio_rx? Because how would the application look like? Maybe you can add tests that show how to enable/disable tx and/or rx mode of the uart?

#else
(void)buffer;
(void)count;
return -ENOTSUP;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just wondering if this has any side effects on existing applications?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Difficult question... Maybe returning count is the better choice...

@dylad
Copy link
Copy Markdown
Member

dylad commented Nov 9, 2017

I think we need a bit of documentation somewhere to explain how disables uart_stdio_rx and why, as it not very straightforward right now.

@roberthartung
Copy link
Copy Markdown
Member

@dylad +1 ... go ahead ;) And also we should give a short overview, where uart is initialized. For stm32 it is in newlib, am I right here? But for other platforms it is in the board_init, e.g. telosb, z1 and a few others (including my platform ;)).

@tcschmidt
Copy link
Copy Markdown
Member

Are we waiting for documentation here? This is probably the duty of @kaspar030

@tcschmidt
Copy link
Copy Markdown
Member

@yegorich can you comment on this PR?

@yegorich
Copy link
Copy Markdown
Contributor

I've looked at various users of stdio_read(). Some users check the return value like newlib's syscalls.c, but others atmega_stdio.c or msp430_stdio.c just return a theoretically received character without checking stdio_read's return value.

I wonder whether it make sense to propagate MODULE_UART_STDIO_RX to cpu files, so that it looks like in cpu/fe310/nano/nanostubs.c :

_ssize_t _read(int fd, void *buffer, size_t count)
  {
  #if defined(MODULE_STDIO_UART)
      if (fd == STDIN_FILENO) {
          return stdio_read(buffer, count);
      }
      errno = EBADF;
      return -1;
  #else
      (void) fd;
      (void) buffer;
      (void) count;
      errno = ENODEV;
      return -1;
  #endif
  }

@smlng
Copy link
Copy Markdown
Member

smlng commented Dec 19, 2018

@MrKevinWeiss maybe something you would like to have a look at, too?

@kaspar030
Copy link
Copy Markdown
Contributor Author

I'm not gonna work on this in the foreseeable future. Anyone interesting in taking over?

@vincent-d
Copy link
Copy Markdown
Member

@kaspar030 I rebased your branch to a recent master, do you mind if I push it here, or do you prefer I open a new PR?

@kaspar030
Copy link
Copy Markdown
Contributor Author

Closing in favor of #11310.

@kaspar030 kaspar030 closed this Apr 1, 2019
@kaspar030 kaspar030 deleted the make_uart_stdio_rx_optional branch April 1, 2019 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: pm Area: (Low) power management Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants