Skip to content

Conversation

@jakub-uC
Copy link
Contributor

@jakub-uC jakub-uC commented Oct 4, 2018

Added functionality to enable active shell backends via Kconfig
file. When there will be more backends implemented user will
have an option to select only required ones.

It is no longer needed to select SERIAL in prj.conf.

Fixes #10190

Signed-off-by: Jakub Rzeszutko [email protected]

@jakub-uC jakub-uC force-pushed the shell_fixing_Kconfig_dependencies branch from 8efaf14 to b0a29f0 Compare October 4, 2018 13:26
Copy link
Contributor

Choose a reason for hiding this comment

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

No quotes needed around the help texts (they become part of it if added).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here re. quotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Could add a space after #

Copy link
Contributor

Choose a reason for hiding this comment

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

depends on is usually a better choice than select when visible symbols (symbols with prompts) are involved.

See #10212 (comment) for discussion. It involves the same symbol.

Copy link
Member

Choose a reason for hiding this comment

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

About this, please note that #10190 got reopened due to this:
#10127 (comment)
#10127 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, had missed the ping in #10190.

Just default y if SHELL on SERIAL would be problematic if SHELL needs SERIAL, as it would still be possible for the user to turn off SERIAL with SHELL enabled in that case.

I won't block this for select-related issues at least.

@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #10379 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10379      +/-   ##
==========================================
- Coverage    52.9%   52.83%   -0.07%     
==========================================
  Files         210      210              
  Lines       25732    25732              
  Branches     5672     5672              
==========================================
- Hits        13614    13596      -18     
- Misses       9802     9821      +19     
+ Partials     2316     2315       -1
Impacted Files Coverage Δ
lib/posix/clock.c 33.33% <0%> (-38.89%) ⬇️
lib/posix/timer.c 62.65% <0%> (-4.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 316f860...83f6e28. Read the comment docs.

nashif
nashif previously requested changes Oct 4, 2018
@nashif nashif dismissed their stale review October 4, 2018 19:30

addressed

Added functionality to enable active shell backends via Kconfig
file. When there will be more backends implemented user will
have an option to select only required ones.

It is no longer needed to select SERIAL in prj.conf.

Fixes zephyrproject-rtos#10190

Signed-off-by: Jakub Rzeszutko <[email protected]>
@jakub-uC jakub-uC force-pushed the shell_fixing_Kconfig_dependencies branch from b0a29f0 to 83f6e28 Compare October 4, 2018 20:58
@carlescufi carlescufi merged commit bc6da1c into zephyrproject-rtos:master Oct 5, 2018
@jakub-uC jakub-uC deleted the shell_fixing_Kconfig_dependencies branch January 30, 2019 15:04
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

Successfully merging this pull request may close these issues.

7 participants