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.

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

Stream.h missing from mbed.h #13354

Closed
sstaub opened this issue Jul 25, 2020 · 13 comments · Fixed by #14212
Closed

Stream.h missing from mbed.h #13354

sstaub opened this issue Jul 25, 2020 · 13 comments · Fixed by #14212

Comments

@sstaub
Copy link
Contributor

sstaub commented Jul 25, 2020

Description of defect

Stream.h is missing in mbed.h
This cause problems with libraries using the Stream classes, this problems comes up with v6.x
Before the Stream.h was included in mbed.h.

Target(s) affected by this defect ?

STM32 Nucleo-F767ZI

Toolchain(s) (name and version) displaying this defect ?

PlatformIO ST STM32

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.2.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

toolchain-gccarmnoneeabi @ 9.2.1

How is this defect reproduced ?

#include TextLCD.h HD44780 library https://os.mbed.com/components/HD44780-Text-LCD/
class TextLCD : public Stream gives following error not a class or struct name
because Stream.h is missing

@ciarmcom
Copy link
Member

@sstaub thank you for raising this issue.Please take a look at the following comments:

What target(s) are you using?
What toolchain(s) are you using?
We cannot automatically identify a release based on the version of Mbed OS that you have provided.
Please provide either a single valid sha of the form #abcde12 or #3b8265d70af32261311a06e423ca33434d8d80de
or a single valid release tag of the form mbed-os-x.y.z .
E.g. '6.2' has not been matched as a valid tag or sha.It would help if you could also specify the versions of any tools you are using?
How can we reproduce your issue?

NOTE: If there are fields which are not applicable then please just add 'n/a' or 'None'.This indicates to us that at least all the fields have been considered.
Please update the issue header with the missing information, the issue will not be mirroredto our internal defect tracking system or investigated until this has been fully resolved.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 27, 2020

I can't find a pull request that would remove Stream.h , was it included previously ?

My assumption would be this removed it via classes that depend on Stream.h: #12410 . It was a clean up of deprecated functionality.

I can see USB drivers are using Stream.h.

@kjbracey-arm @rajkan01 do you remember any detail about Stream class

@0xc0170 0xc0170 changed the title Stream.h Stream.h missing from mbed.h Jul 27, 2020
@rajkan01
Copy link
Contributor

rajkan01 commented Jul 27, 2020

I can't find a pull request that would remove Stream.h , was it included previously ?
My assumption would be this removed it via classes that depend on Stream.h: #12410 . It was a clean up of deprecated functionality.
I can see USB drivers are using Stream.h.
@kjbracey-arm @rajkan01 do you remember any detail about Stream class

Yes, previously Stream.h header included via drivers/Serial.h header file. As part of the cleanup, the deprecated class was removed and Serial.h the header in #12410.
I am not sure why that header file was not included directly in mbed.h header file instead

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 27, 2020

I am not sure why that header file was not included directly in mbed.h header file instead

A missed bit. If the class is used (it is in USB drivers) so should be exposed, shouldn't be?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 16, 2020

@rajkan01 worth fixing ? We can send a pull request

cc @adbridge if you got a chance to review this and fix?

@ciarmcom
Copy link
Member

ciarmcom commented Oct 2, 2020

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2138

@ARMmbed ARMmbed deleted a comment from ciarmcom Oct 6, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 26, 2021

@adbridge Can you look at this one?

@adbridge
Copy link
Contributor

@rajkan01 So if stream.h was removed because previously it was in Serial.h , shouldn't stream.h instead be included in the replacement for Serial.h ? ie bufferedserial.h and/or unbufferedserial.h ?

@adbridge
Copy link
Contributor

@sstaub while we debate if / where Stream.h should sit, one point to note is that the TextLCD you are using is actually from Mbed 2 and will have not been tested / ported to Mbed 5 or Mbed 6 so there are no guarantees of it working...

@sstaub
Copy link
Contributor Author

sstaub commented Jan 28, 2021

I have made some LCD libraries which are updated for Mbed 6 I published also on my Mbed site, I included the Stream.h manually, but I think this class should included in mbed.h

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 28, 2021

Thanks for the feedback, it looks to me it was previously exposed as @rajkan01 wrote and it's public API:

#ifndef MBED_STREAM_H
#define MBED_STREAM_H

#include "platform/platform.h"
#include "platform/FileLike.h"
#include "platform/NonCopyable.h"
#include "platform/mbed_toolchain.h"
#include <cstdio>
#include <cstdarg>

namespace mbed {
/** \addtogroup platform-public-api */
/** @{*/

@kjbracey
Copy link
Contributor

kjbracey commented Jan 28, 2021

I've been pushing for a long time for Stream to be deprecated. It's a helper class that helps you implement the FileHandle API, but it does so very badly. I'd much rather people implemented FileHandle directly than use Stream to do it.

Here was my attempt to do that deprecation: #5655

I didn't manage to win the debate then.

We've since replaced Serial, along the lines first discussed in that PR, but Stream remains. And it's no better than it was then.

Previously it would have been picked up accidentally by mbed.h getting it from Serial.h. As it is still public API, mbed.h should be including it explicitly now for anyone using it.

I'd still like to deprecate it though.

@sstaub
Copy link
Contributor Author

sstaub commented Jan 28, 2021

Sorry, this absolut ugly. Does anybody from ARM read the forum? Most people are not happy about the way Mbed goes.
How should we implement an easy printf functionality for libraries? You APIs get more and more stupid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants