search

lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
45 stars 52 forks source link

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

Open martinwork opened 2 months ago

martinwork commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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. https://github.com/lancaster-university/codal-core/commit/c918ffb562722c5892e600a1c750c78e59a322a3#diff-d81b0f05e820e45bd7f55ced2d2663eb76b21de23148f3679abfa38362ad9ba3L192 https://github.com/lancaster-university/codal-core/commit/3b59df593aea030ac452c580c9f1499c514180fc#diff-d81b0f05e820e45bd7f55ced2d2663eb76b21de23148f3679abfa38362ad9ba3L193