Skip to content

Stream::write and Stream::read are not public #9338

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.

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

Closed
seppestas opened this issue Jan 10, 2019 · 5 comments
Closed

Stream::write and Stream::read are not public #9338

seppestas opened this issue Jan 10, 2019 · 5 comments

Comments

@seppestas
Copy link
Contributor

Description

Mbed features the Stream class. This class provides "streaming" functionality to FileLike drivers like e.g the Serial driver, allowing them to use things like printf and scanf.

Using the Stream class in device drivers for e.g a UART device offers a lot of flexibility vs using the overloading class like the Serial class since it allows to use the same driver for multiple types of drivers. E.g a device that is e.g connected to a SPI to UART bridge instead of directly to a UART port could be used very easily.

The Stream class requires the underlying driver to implement the write and read functions, but since these functions are private, only the printf, putc, scanf, gets and getc(and va_list alternatives) functions can be used. The printf, scanf and getsfunctions rely on C strings, making them useless for devices requiring a binary interface. This means these devices are stuck using character-per-character reads and writes, even if the underlying driver supports more efficient bulk transfer methods. Also, the printf and scanf functions come with a lot of overhead (see #4830).

Making the read and write functions public would allow the Steam interface to be used without losing the ability to do efficient bulk transfers.

Issue request type

[ ] Question
[x] Enhancement
[ ] Bug
@seppestas seppestas changed the title Stream::write and stream::read are not public Stream::write and Stream::read are not public Jan 10, 2019
@ciarmcom
Copy link
Member

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-409

@kjbracey
Copy link
Contributor

I really do not recommend using Stream, and you've listed many of the reasons. If implementing a device, it's usually better to implement FileHandle directly.

FileHandle is the abstraction point, and Stream is just a "helper" to make a FileHandle from a simple character device, but it is very limited, and in some ways just broken (eg its read method). There is no code taking Stream as an abstract type, but there is a lot taking FileHandle.

Documentation about FileHandle is in progress here: ARMmbed/mbed-os-5-docs#745

On the subject of Stream::write not being public - it actually accessible, because it's a virtual method that's public in the base class FileHandle. So this works:

 Stream *stream;
 FileHandle *file = stream;
 file->write(...);

The problem is just Stream's character-at-a-time implementation. If you implement FileHandle directly you can do better.

The Stream class requires the underlying driver to implement the write and read function

No, it requires the underlying driver to implement _getc and _putc. It then implements write and read on top of those. (Incorrectly, for read).

I did try deprecating Stream here: #5655 - you may find discussion there useful.

@seppestas
Copy link
Contributor Author

I really do not recommend using Stream, and you've listed many of the reasons. If implementing a device, it's usually better to implement FileHandle directly.

The reason I used Stream is mainly convenience. Especially for "Ascii character" devices like modems having printf functionality is great. I guess a good way of handeling this would be to create a stream from a FileHandle? Apart from that, Stream just sounds like a more logical device for me than FileHandle, especially when dealing with things like UART and SPI.

I see that the read and write functions indeed rely on _getc and _putc, which is kind of a shame. Why doesn't it use file_read and file_write. Also, would there actually be a benefit to this?

I feel like an abstraction for async and/or buffered reads and writes is needed to properly support what I'm looking for. Right now it seems like the only option is using the Serial driver directly, limiting portability of libraries.

@seppestas
Copy link
Contributor Author

seppestas commented Feb 20, 2019

Closing this since using the Stream class directly indeed seems to be an anti-pattern.

@kjbracey
Copy link
Contributor

I missed your last comment - you should be able to achieve everything you need via FileHandle, but it may just need an extra step. printf on a character device is

 FileHandle *device;
 FILE *stream = fdopen(&device, "w");
 fprintf(stream, "Hello\n");

Although convenient, having something as complex as printf as a method is a modern C++ anti-pattern - printf does not need any access to the internals of the object. It's an "algorithm" to apply to an object that provides simpler functionality (read and write). It's not really scalable to make an object have all possible algorithms as methods - keep algorithms external.

C++ core guidelines reference: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-member

I'm also fascinated by the suggestion linked to there by Stroustrup and Sutter, which I'd not seen before: Unified Call Syntax. That would eliminate any visible difference between methods and non-methods to the user in general (although this particular case isn't quite so simple, due to C compatibility issues)..

Generic code can and should use FileHandle as their abstraction point - you'll see examples in ATCmdParser, the PPP driver code for lwIP and the C library and console retargeting code.

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

No branches or pull requests

3 participants