lancaster-university / codal-microbit-v2

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

Add `setSampleRange()` to `StreamSplitter` to match `Mixer2::setSampleRange()` #393

Open dpgeorge opened 10 months ago

dpgeorge commented 10 months ago

It's currently possible to set the sample range for output streams (producing sound on the speaker) using Mixer2::setSampleRange(), and also in the addChannel() method as the third argument.

We use this in MicroPython to set the sample range to 255, which, in combination with DATASTREAM_FORMAT_8BIT_UNSIGNED, allows samples to be in the range 0-255, with a silent/idle value of 128.

But when recording sound from the microphone it doesn't seem possible to configure the sample range, and using DATASTREAM_FORMAT_8BIT_UNSIGNED as the format still results in samples that range -128 to 127.

So I propose adding StreamSplitter::setSampleRange(), and/or the ability to pass in the sample range to StreamSplitter::createChannel() when creating the channel.

JohnVidler commented 10 months ago

The StreamNormalizer already has the functionality for this - if you create a new Normalizer attached to the incoming mic stream from the splitter, you can alter the stream format to any other one: https://github.com/lancaster-university/codal-core/blob/master/inc/streams/StreamNormalizer.h#L71C92-L71C92

static SplitterChannel *splitterChannel = uBit.audio.splitter->createChannel();
static StreamNormalizer *formatter = new StreamNormalizer( *splitterChannel, 1.0f, true, DATASTREAM_FORMAT_8BIT_SIGNED );
static StreamRecording *recording = new StreamRecording( *formatter );

Doing it this way also gives you a per-channel gain control, for local volume changes.

dpgeorge commented 9 months ago

OK, thanks for that, I didn't know about StreamNormalizer.

Actually, it depends on the outcome of #395, how this issue here will be solved. For example it may be that StreamNormalizer needs pullInto() added to it. Or after calling SplitterChannel::pullInto() I can do a quick in-place pass over the returned data to normalize/adjust it accordingly.

microbit-carlos commented 7 months ago

@dpgeorge does the resolution of https://github.com/lancaster-university/codal-microbit-v2/issues/395 and the solution mentioned here cover the MicroPython use case? Are we happy to close this as resolved?

dpgeorge commented 7 months ago

does the resolution of #395 and the solution mentioned here cover the MicroPython use case? Are we happy to close this as resolved?

MicroPython's use-case is still not satisfied, because switching to SplitterChannel::pullInto() means that it's not possible to use StreamNormalizer, and so we need to manually adjust the incoming data to change the range from -128-127 to 0-255.

This is not really a problem because it's a pretty simple loop to make this range adjustment. Using a StreamNormalizer if it had pullInto() would probably be extra overhead, so I wouldn't really want to use it even if it did exist.

So, I'm happy to close this as "not needed".

JohnVidler commented 7 months ago

Technically if very specific formats and rates are needed on a channel, it is possible to chain: uBit.audio.splitter -> normalizer -> splitter -> user code with minimal overhead, as both the normalizer and splitter are both pretty lean on RAM use, and both will already be in the code block as we use them by default.

However, it may be possible to just set the format up on the uBit.audio.processor (the system-level StreamNormaliser) via the setFormat with normalise enabled and it should start emitting the requested data format, and I believe that other components will just deal with it...

However, that'll only work without loss if we stick to 8-bit sample ranges, (signed/unsigned), because up at the microphone we already scale the output samples to that range, anything larger will just lose information in a rescale operation. So more generally we should provide a mechanism to set the microphone properties in similar/identical ways to the mixer, ideally.

Personally I think its time that the microphone driver is actually split out to be a component attached to an arbitrary ADC, then it can have any combination of parameters set, and we can multiplex the ADC into multiple virtual 'microphone's if we need very specific properties on each. This will need the driver(s) to share control of the microphone power lines, but that's not too tricky to implement.

@microbit-carlos and @finneyj I'd love to hear your thoughts on this - it would mostly just involve a refactor of some portion of MicroBitAudio with the mic flow control code being bumped up to its own class. We can proxy all the existing calls to this new class, such that the only visible change would be the type of the mic* pointer, which would presumably become the new default Microphone class instance.

I don't think this would break any downstream code, but its worth us checking as well.

finneyj commented 7 months ago

Thanks @JohnVidler.

Personally I'd keep it simple. There'll be a copy operation into the micropython allocated memory at some point anyway (unless we unify the heap allocators, as we do in MakeCode). So simplest just to accept a standard ManagedBuffer into the micropython side shim code and then copy there.

I'm not quite sure why Damien's attempt to get an unsigned 8 bit buffer didn't work though. That sounds like it could be a bug? In which case we could just squash that bug and declare success?

JohnVidler commented 7 months ago

@dpgeorge Do you have some code I can take a look at for where this falls over? Then I can perhaps come up with a CODAL MWE for you?

dpgeorge commented 6 months ago

Do you have some code I can take a look at for where this falls over? Then I can perhaps come up with a CODAL MWE for you?

@JohnVidler you'll need to check out the audio-recording branch here: https://github.com/microbit-foundation/micropython-microbit-v2/tree/audio-recording

Then look at the file src/codal_app/microbithal_microphone.cpp. You can see a line that does splitterChannel->setFormat(DATASTREAM_FORMAT_8BIT_UNSIGNED); but that's not enough to get it to work. I also need to add the following loop to MyStreamRecording::pullRequest():

        // Convert signed 8-bit to unsigned 8-bit data.
        for (size_t i = 0; i < n; ++i) {
            pull_buf[i] += 128;
        }

That's a pretty simple loop. That loop could be removed if the conversion was done somewhere/somehow by CODAL.

JohnVidler commented 5 months ago

From the Codal Sync today, we suggest that we promote a new global variable for the system-default audio format, so that it can be updated as part of the build config, but we also have a setFormat() call on the uBit.audio.processor pointer to make a runtime, global change to the pipeline format.

If this is changed at runtime you might get a bit of a 'bump' from the level detector at the point of the change, but everything else should be happy with the format change.

I'll create the above global config variable for the format on startup, but could you (@dpgeorge ) please let us know if there are any strange effects on any other part of the pipeline after making the change in format.