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.

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

cpu/common/esp8266: use 'awk/printf' instead of 'echo' #12850

Merged
merged 1 commit into from
Dec 1, 2019
Merged

cpu/common/esp8266: use 'awk/printf' instead of 'echo' #12850

merged 1 commit into from
Dec 1, 2019

Conversation

LordTy
Copy link
Contributor

@LordTy LordTy commented Dec 1, 2019

Contribution description

This is a fix for the same issue encountered with the esp32 in #12244
With the new esp toolchain from #11108, this breaks on OSX.

So this changes the uses of "echo -n" to printf, in the same way as #12282

Testing procedure

Same as #12282

Issues/PRs references

Fixes issue #12244 for the esp8266

@bergzand bergzand requested a review from gschorcht December 1, 2019 13:43
@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Dec 1, 2019
gschorcht
gschorcht reviewed Dec 1, 2019
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Many echo implementations are not compliant with the Unix specification which doesn't allow option processing for the echo command. Therefore, the echo command is considered to be non-portable command and the printf command is prefered. Thus, this change makes sense.

@gschorcht gschorcht added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Dec 1, 2019
gschorcht
gschorcht approved these changes Dec 1, 2019
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Tested. ACK

@gschorcht gschorcht merged commit 0209afd into RIOT-OS:master Dec 1, 2019
@gschorcht
Copy link
Contributor

@LordTy Thanks for figuring out the problem and providing the fix.

@LordTy
Copy link
Contributor Author

LordTy commented Dec 1, 2019

Thanks!

@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht approved these changes

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
4 participants
@LordTy @gschorcht @bergzand @fjmolinas