-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Extend Stream to provide remove(count) and peek(offset) #17
Conversation
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).
I'd like this, could have used this the other day. |
This would really help out an application I'm developing. |
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 |
There was a problem hiding this comment.
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;
.
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? |
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/ |
There was a problem hiding this comment.
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.
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? |
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. :-)