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

StreamRecording::canPull() does not correspond to it's description in the code comments. #453

Open
martinwork opened this issue Sep 23, 2024 · 2 comments

Comments

@martinwork
Copy link
Collaborator

StreamRecording::canPull() (not used) does not correspond to it's description in the code comments.

https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/inc/streams/StreamRecording.h#L91

It's documented to tell downstream if there is data to pull, but actually returns if it has room to pull from the upstream (== !isFull())

https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/source/streams/StreamRecording.cpp#L29

Compare FIFOStream::canPull()
https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/source/streams/FIFOStream.cpp#L28

To work as documented, it should perhaps return this->readHead != NULL.

@microbit-carlos
Copy link
Collaborator

Thanks Martin!

I had a look in micropython-microbit-v2, pxt, pxt-microbit, and pxt-common-packages, and it doesn't seem to be used by MakeCode nor MicroPython either.

The DataSink::canPull(size) version (which is only used in Synthesizer::idleCallback()) is also slightly different:
https://github.com/lancaster-university/codal-core/blob/509086cc8590465041b15493ab52b56e7071c110/inc/streams/DataStream.h#L172-L178

@finneyj We should probably ensure the behaviour is as close as possible in all versions.

@martinwork
Copy link
Collaborator Author

martinwork commented Oct 2, 2024

Ah yes, comparing with DataStream::canPull, maybe it's the description of StreamRecording::canPull() that's inconsistent.

FIFOStream doesn't have a description, and implements StreamRecording's description.

DataStream used to have full(), and canPull (can a buffer be added). full() was first commented out, then deleted.
lancaster-university/codal-core@c918ffb#diff-d81b0f05e820e45bd7f55ced2d2663eb76b21de23148f3679abfa38362ad9ba3L192
lancaster-university/codal-core@3b59df5#diff-d81b0f05e820e45bd7f55ced2d2663eb76b21de23148f3679abfa38362ad9ba3L193

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

No branches or pull requests

2 participants