Skip to content

Extend Stream to provide remove(count) and peek(offset) #17

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

Conversation

blalor
Copy link

@blalor blalor commented Jan 8, 2011

I needed to extend HardwareSerial to get more access to the internal buffer; for my project, it was wasteful to have two statically-allocated buffers for incoming serial data, but I needed to be able read arbitrary bytes of the receive buffer. This is the result. The comments on the individual commits should make it pretty clear what was done. :-)

I need more access to HardwareSerial's RX buffer; peek()'s a great step
forward, but I need to be able to peek at an arbitrary byte, and quickly
remove a number of bytes.

• Adding peek(uint8_t) and remove(uint8_t) to Stream interface.
• Provided default implementation to minimize (and hopefully eliminate) impact
  on other Stream implementations.
• Implemented peek(uint8_t) and remove(uint8_t) in HardwareSerial.
• Moved RX_BUFFER_SIZE to HardwareSerial.h so it's accessible to others.
Declared Stream's default implementations of peek(uint8_t) and remove(uint8_t)
to be virtual, so that they can be appropriately overridden by extending
classes.

I believe HardwareSerial's header should only declare member functions to be
virtual if it's going to be further extended, and that's not currently
possible in the current Arduino codebase (the user has no ability to intercept
the creation of HardwareSerial instances, something I hope to rectify soon).
@LJNielsenDk
Copy link

I'd like this, could have used this the other day.

@PatrickPijnappel
Copy link

This would really help out an application I'm developing.

@ffissore ffissore added the New label Feb 27, 2014
@cmaglie cmaglie added Component: Core Related to the code for the standard Arduino API feature request A request to make an enhancement (not a bug fix) labels Apr 8, 2015
virtual void flush(void);
virtual void write(uint8_t);

// @todo I think these only need to be declared virtual if HWSerial's going to be extended
Copy link
Contributor

Choose a reason for hiding this comment

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

No, virtual is not needed.

If a derived object implements peek() this is the one the virtual methods will call. For that object to use the parent versions, it'll have to explicitly call derived::peek().

I wouldn't use virtual for another reason: A client may want to use the derived version without implementing their own using derived::peek;.

@matthijskooijman
Copy link
Collaborator

Should a default peek() implementation also be provided (calling peek(0))? That allows subclasses to only implement peek(count) and eventually perhaps making peek() non-virtual. The downside of this is that if a subclass implements neither peek method, it will end up as infinite recursion instead of a compiler error, but that's probably acceptable?

@NicoHood
Copy link
Contributor

The passed variable type should be int or size_t instead of uint8_t. I'd go for size_t.

Also I'd remove the peek() virtual function and make it a normal function that calls peek(0). Libraries might need to add using::peek(); or something like that. Using too many virtual functions adds too much overhead. If we can avoid it, please do so.

I'd also add a function that resets the buffer (discards all bytes).

@@ -0,0 +1,6 @@
app/bin/
Copy link
Contributor

Choose a reason for hiding this comment

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

And this gitignore file is really not needed.

@facchinm facchinm added the Print and Stream class The Arduino core library's Print and Stream classes label Jan 20, 2017
@facchinm
Copy link
Member

Hi @blalor , I know this PR is more than 8 years old (forgive us if this happened) but due to reorganization the patch should be moved to https://github.com/arduino/ArduinoCore-api . Is it still something that you (or anyone else) might be interested to discuss?
In the meantime, I'm closing the issue since it doesn't belong here anymore. To anyone interested, please open a new PR/issue on ArduinoCore-api linking this one, thanks.

@facchinm facchinm closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Related to the code for the standard Arduino API feature request A request to make an enhancement (not a bug fix) Print and Stream class The Arduino core library's Print and Stream classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants