Convert audiosamples to use micropython "protocols" (safely) #2353
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Protocols are nice, but there is no way for C code to verify whether
a type's "protocol" structure actually implements some particular
protocol. As a result, you can pass an object that implements the
"vfs" protocol to one that expects the "stream" protocol, and the
opposite of awesomeness ensues.
This patch adds an OPTIONAL (but enabled by default) protocol identifier
as the first member of any protocol structure. This identifier is
simply a unique QSTR chosen by the protocol designer and used by each
protocol implementer. When checking for protocol support, instead of
just checking whether the object's type has a non-NULL protocol field,
use `mp_proto_get` which implements the protocol check when possible.
The existing protocols are now named:
protocol_framebuf
protocol_i2c
protocol_pin
protocol_stream
protocol_spi
protocol_vfs
(most of these are unused in CP and are just inherited from MP; vfs and
stream are definitely used though)
I did not find any crashing examples, but here's one to give a flavor of what
is improved, using `micropython_coverage`. Before the change,
the vfs "ioctl" protocol is invoked, and the result is not intelligible
as json (but it could have resulted in a hard fault, potentially):
>>> import uos, ujson
>>> u = uos.VfsPosix('/tmp')
>>> ujson.load(u)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: syntax error in JSON
After the change, the vfs object is correctly detected as not supporting
the stream protocol:
>>> ujson.load(p)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OSError: stream operation not supported
This eases addition of new sample sources, since the manual virtual function dispatch functions are just calls via a protocol
|
I'd mildly like this for the mp3 PR but that's no reason to hurry it. |
tannewt
requested changes
Dec 4, 2019
tannewt
reviewed
Dec 6, 2019
shared-module/audiocore/__init__.h
Outdated
Comment on lines
42
to
49
| typedef uint32_t (*audiosample_sample_rate_fun)(void* sample_obj); | ||
| typedef uint8_t (*audiosample_bits_per_sample_fun)(void* sample_obj); | ||
| typedef uint8_t (*audiosample_channel_count_fun)(void* sample_obj); | ||
| typedef void (*audiosample_reset_buffer_fun)(void* sample_obj); | ||
| typedef audioio_get_buffer_result_t (*audiosample_get_buffer_fun)(void* sample_obj, | ||
| bool single_channel, uint8_t channel, uint8_t** buffer, | ||
| uint32_t* buffer_length); | ||
| typedef void (*audiosample_get_buffer_structure_fun)(void* sample_obj, |
There was a problem hiding this comment.
These sample_objs can also be mp_obj_t right? That'd make it clearer that the first field is the Python type pointer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Audio sample sources are essentially objects with virtual methods. However, the existing approach is to have dispatch functions that just check for each statically known type of object in turn. Using the underlying "protocol" facility of Micropython enables use of function pointers instead, so that the dispatch function need not know about all the sample types. This will make it slightly easier to add mp3 playback as a new audio sample source.
Protocols are nice, but before now there was no way for C code to verify whether a type's "protocol" structure actually implements some particular protocol. As a result, you could pass an object that implements the "vfs" protocol to one that expects the "stream" protocol, and the opposite of awesomeness ensues.
This patch adds an OPTIONAL (but enabled by default) protocol identifier as the first member of any protocol structure. This identifier is simply a unique QSTR chosen by the protocol designer and used by each protocol implementer. When checking for protocol support, instead of just checking whether the object's type has a non-NULL protocol field, use
mp_proto_getwhich implements the protocol check when possible.The existing protocols are now named:
protocol_framebuf
protocol_i2c
protocol_pin
protocol_stream
protocol_spi
protocol_vfs
(most of these are unused in CP and are just inherited from MP; vfs and stream are definitely used though)
I did not find any crashing examples, but here's one to give a flavor of what is improved, using
micropython_coverage. Before the change, the vfs "ioctl" protocol is invoked, and the result is not intelligible as json (but it could have resulted in a hard fault, potentially):After the change, the vfs object is correctly detected as not supporting the stream protocol:
Testing performed: UNIX testsuite; played a rawsample on an nRF PWMAudioOut