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

out_stream option to simplify connecting up a Loopback/PipedStream to an external Stream #7

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

embedded-creations
Copy link

@embedded-creations embedded-creations commented Oct 7, 2021

Hi, I've been making extensive use of your library over the past few months, working on improvements to the Commander Library, creating a new library that connects multiple Streams (including File objects) between two Arduinos over a single Stream object, and adapting tinyproto's error checking and retransmit functionality to act as a Stream. One thing I needed was the ability to buffer Stream data that is eventually going to be sent to another Stream object, so I added support for that to ArduinoBufferedStreams. I added documentation for the new features in the form of examples.

I like to keep commits as small logical blocks, but you should be able to review this PR by looking at the "Files Changed" tab to see all the changes at once instead of individual commits. If you'd prefer I squashed the commits, I can do that, just need to figure out how.

If you don't want to merge this commit or think the merge might take a long time, please let me know so I can figure out what to do instead, as my other projects that I want to release soon depend on my changes in your library.

Your code was a big help to me early on in understanding how Stream objects work and how to convert data to/from Streams, so thanks for that!

@embedded-creations embedded-creations changed the title out_serial option to simplify connecting up a Loopback/PipedStream to an external Stream out_stream option to simplify connecting up a Loopback/PipedStream to an external Stream Oct 7, 2021
@paulo-raca
Copy link
Owner

Hello!

Thank you for the contribution, I like it a lot! ❤️

However I think these changes modify what a LoopbackStream / PipedStream are designed to do.

Instead, I suggest to add the new functionality in new classes: For example we could add a BufferedStream to implement the LoopbackStream + out_stream you want.

What do you think?

@embedded-creations
Copy link
Author

embedded-creations commented Oct 11, 2021

I'm open to that idea. If I understand you correctly, BufferedStream would be derived from LoopbackStream, and add the new out_stream functionality. We'd then need the equivalent for PipedStream, would it be named BufferedPipedStream, and contain two BufferedStream objects?

It seems to me like it's more work to separate the classes and maintain them separately than to add some optional functionality to the existing Loopback and Piped classes. The additional functionality doesn't seem to me to add much overhead to the existing classes.

@embedded-creations
Copy link
Author

Hi @paulo-raca, any thoughts on the above?

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

Successfully merging this pull request may close these issues.

2 participants