Skip to content

Complete mbed_retarget FileHandle rework #5571

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 4 commits into from
Feb 7, 2018
Merged

Complete mbed_retarget FileHandle rework #5571

merged 4 commits into from
Feb 7, 2018

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Nov 23, 2017 •
edited
Loading

To complete the move to POSIX-ness and FileHandle-ness, close the loop by reworking mbed_retarget.cpp so that everything is a FileHandle.

This adds:

  • POSIX file APIs - open/close/read/write/lseek/isatty/fsync/fstat/fdopen/poll.
  • The ability to have buffered stdin/stdout using UARTSerial with a simple JSON change.
  • The ability for target or application to change stdin/stdout/stderr to be a custom FileHandle by overriding weak functions..
  • The option of CRLF conversion for any isatty() stream, not just stdin/stdout/stderr.

@kjbracey
Copy link
Contributor Author

@SeppoTakalo, @artokin, @geky, @sg-, @c1728p9, @pan-

This was prompted by #5356 - it achieves the same result in a more elegant fashion. The other reason for it is to make it easy to improve performance by making stdout be buffered, rather than spinning waiting for the serial port.

@kjbracey kjbracey mentioned this pull request Nov 23, 2017
Closed
@kjbracey
Copy link
Contributor Author

FYI @tommikas

@kjbracey kjbracey force-pushed the retarget_fh branch 8 times, most recently from 68d1231 to 1a4f881 Compare November 27, 2017 09:19
@kjbracey kjbracey mentioned this pull request Dec 5, 2017
Closed
This was referenced Dec 19, 2017
Closed
Closed
@TeroJaasko
Copy link
Contributor

+2. Overriding standard output is a recurring theme, recently there have been at least two PRs related to it. This PR would ease the effort on customer and remove need for patching Mbed OS locally.

@adbridge
Copy link
Contributor

@sg- You may wish to take a look at this...

@bmcdonnell-ionx
Copy link
Contributor

I'm interested in stat(), fsync(), and fdatasync(), particularly for use with an SD Card (i.e. FATFileSystem on SDBlockDevice).

Is there a target mbed OS version # for having this completed and merged?

@cmonr cmonr requested a review from sg- January 12, 2018 19:59
@cmonr
Copy link
Contributor

cmonr commented Jan 12, 2018

Adding @sg- as a reviewer, since he's also taken a look at #5356.

@kjbracey-arm, if I understand correctly, is this intended to supersede #5356, or complement it?

@geky
Copy link
Contributor

geky commented Jan 12, 2018 •
edited
Loading

Sorry I haven't been able to give this a good review yet. From a high-level it looks good though!

One big request: Would it be possible to add tests that check if we're at least retargeting these functions correctly? (A dummy filesystem that sets a flag might be the easiest method). This would also catch conflicting function definitions with the toolchains.

Making sure the retargeting stuff actually works on all of the toolchains is a big hole that may become a problem.

@kjbracey
Copy link
Contributor Author

@cmonr, yes, this would supersede #5356

@geky, yes I can look at some tests. (Not looked into what tests already exist yet.)

@kjbracey kjbracey force-pushed the retarget_fh branch 2 times, most recently from bd0c523 to ccae00f Compare January 17, 2018 11:01
@kjbracey
Make stdin/stdout/stderr be FileHandles
f79bfdd 
Rework so that everything is a FileHandle, including
stdin/stdout/stderr.

Provide legacy functionality of calling serial_getc and serial_putc as
an internal "DirectSerial" FileHandle.

Add a JSON option to use UARTSerial instead.

Add hooks for target and application to provide custom FileHandles.

Allow for CRLF conversion to work on any FileHandle that isatty(),
as stdin/stdout or any other FILE. Optimise the conversion so it
doesn't force all write calls to be 1 byte. Limit the conversion
to the stdio layer, so that read() and write() work the same as
the FileHandle methods - this seems less confusing.
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Feb 6, 2018

Build : SUCCESS

Build number : 1069
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5571/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Feb 6, 2018

Test : SUCCESS

Build number : 877
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5571/877

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2018

/morph uvisor-test

@mbed-ci
Copy link

mbed-ci commented Feb 6, 2018

Exporter Build : SUCCESS

Build number : 747
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5571/

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2018

/morph uvisor-test

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2018

Locally tested this PR with #5895, green for all compilers (used k64f target)

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 7, 2018

tests were integrated, restarting CI

/morph build

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2018

Build : SUCCESS

Build number : 1081
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5571/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2018

Exporter Build : SUCCESS

Build number : 758
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/5571/

@mbed-ci
Copy link

mbed-ci commented Feb 7, 2018

Test : SUCCESS

Build number : 888
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5571/888

@theotherjimmy theotherjimmy merged commit 7b5d48d into ARMmbed:master Feb 7, 2018
@kjbracey kjbracey mentioned this pull request Feb 15, 2018
Closed
@kjbracey kjbracey deleted the retarget_fh branch February 20, 2018 16:46
@maciejbocianski maciejbocianski mentioned this pull request Feb 23, 2018
Merged
4 tasks
@kjbracey kjbracey mentioned this pull request May 2, 2018
Merged
@0xc0170 0xc0170 mentioned this pull request May 4, 2018
Closed
@RobMeades RobMeades mentioned this pull request Jun 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr left review comments

@sg- sg- sg- left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@maciejbocianski maciejbocianski maciejbocianski left review comments

@bmcdonnell-ionx bmcdonnell-ionx bmcdonnell-ionx left review comments

@geky geky geky approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@SenRamakri SenRamakri Awaiting requested review from SenRamakri

Assignees
No one assigned
Labels
release-version: 5.8.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.