Skip to content

Flash shell migrate commands to new shell #10019

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
Merged
merged 1 commit into from
Oct 9, 2018
Merged

Flash shell migrate commands to new shell #10019

merged 1 commit into from
Oct 9, 2018

Conversation

@jakub-uC
Copy link
Contributor

@jakub-uC jakub-uC commented Sep 17, 2018 •
edited
Loading

Migrating flash example to new shell.

Fixes #8883

@nordic-krch nordic-krch mentioned this pull request Sep 17, 2018
Closed
@jakub-uC jakub-uC force-pushed the flash_shell_migrate_commands_to_new_shell branch from 468a4fe to 7baf079 Compare September 17, 2018 12:09
@jakub-uC jakub-uC added the DNM This PR should not be merged (Do Not Merge) label Sep 19, 2018
@jakub-uC jakub-uC force-pushed the flash_shell_migrate_commands_to_new_shell branch from 7baf079 to 4018f0c Compare September 19, 2018 09:16
@codecov-io
Copy link

codecov-io commented Sep 19, 2018 •
edited
Loading

Codecov Report

Merging #10019 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10019   +/-   ##
=======================================
  Coverage   53.14%   53.14%           
=======================================
  Files         210      210           
  Lines       25747    25747           
  Branches     5673     5673           
=======================================
  Hits        13682    13682           
  Misses       9755     9755           
  Partials     2310     2310

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 4db9731...31d0772. Read the comment docs.

@jakub-uC jakub-uC force-pushed the flash_shell_migrate_commands_to_new_shell branch from 4018f0c to 5b49f57 Compare September 19, 2018 14:05
@jakub-uC jakub-uC added area: Shell Shell subsystem and removed DNM This PR should not be merged (Do Not Merge) labels Sep 19, 2018
nordic-krch
nordic-krch requested changes Sep 20, 2018
View reviewed changes
samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

ret != 0 indicates error. this should be PR_ERROR. Message should be something like "flash_erase failed (err:%d)."

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

here message is misleading, better would be "Failed to disable flash protection (err: %d)."

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

"Failed to enable flash protection (err: %d)."

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

here, also messages could be improved.

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

PR_ERROR

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

you can extend struct page_layout_data to have shell handler and use PR_SHELL() here. Even though it is called callback, it is executed in the context of shell command so no worries about using shell_fprintf.

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

maybe that could be simple PR()

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

PR_ERROR and improve message.

Copy link
Contributor Author

@jakub-uC jakub-uC Oct 4, 2018

Choose a reason for hiding this comment

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

done

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

PR_ERROR and improve message

samples/drivers/flash_shell/src/main.c Outdated
Copy link
Contributor

@nordic-krch nordic-krch Sep 20, 2018

Choose a reason for hiding this comment

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

PR_ERROR

@nordic-krch
Copy link
Contributor

nordic-krch commented Sep 20, 2018

Apart from review i have one suggestion. Why not move those commands out from sample to /drivers/flash folder and add kconfig option to enable shell commands for flash driver. Sample would just enable those.

@jakub-uC jakub-uC force-pushed the flash_shell_migrate_commands_to_new_shell branch 3 times, most recently from 90c8d50 to 6526078 Compare October 4, 2018 10:09
@jakub-uC
Copy link
Contributor Author

jakub-uC commented Oct 4, 2018

@nordic-krch : done

samples: flash_shell: migrate example to new shell
31d0772 
Convert flash_shell example to use the new shell API.

Signed-off-by: Jakub Rzeszutko <[email protected]>
@jakub-uC jakub-uC force-pushed the flash_shell_migrate_commands_to_new_shell branch from 6526078 to 31d0772 Compare October 9, 2018 10:48
@carlescufi carlescufi merged commit 3d4c525 into zephyrproject-rtos:master Oct 9, 2018
@jakub-uC jakub-uC deleted the flash_shell_migrate_commands_to_new_shell branch January 30, 2019 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlescufi carlescufi carlescufi approved these changes

@nordic-krch nordic-krch nordic-krch approved these changes

@andrewboie andrewboie Awaiting requested review from andrewboie

@andyross andyross Awaiting requested review from andyross

@jhedberg jhedberg Awaiting requested review from jhedberg

@jukkar jukkar Awaiting requested review from jukkar

@nashif nashif Awaiting requested review from nashif

@pfalcon pfalcon Awaiting requested review from pfalcon

@sjanc sjanc Awaiting requested review from sjanc

@tarunkum tarunkum Awaiting requested review from tarunkum

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@Vudentz Vudentz Awaiting requested review from Vudentz

@ramakrishnapallala ramakrishnapallala Awaiting requested review from ramakrishnapallala

@pizi-nordic pizi-nordic Awaiting requested review from pizi-nordic

Assignees

No one assigned

Labels

area: Shell Shell subsystem

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jakub-uC @codecov-io @nordic-krch @carlescufi