Skip to content

Conversation

stegu
Copy link

@stegu stegu commented Jun 21, 2022

What This PR Changes

The documentation was incomplete and vague. I made it better.
This still needs a proper tutorial on how to use buffered writes and reads.

Contribution Guidelines

The documentation was incomplete and vague. I made it better.
This still needs a proper tutorial on how to use buffered writes and reads.
@CLAassistant
Copy link

CLAassistant commented Jun 21, 2022

CLA assistant check
All committers have signed the CLA.

@marqdevx marqdevx added fix/update A small fix or update community Bugs and fixes suggested by the community Tutorial labels Jun 22, 2022
@jhansson-ard jhansson-ard requested a review from marqdevx June 29, 2022 07:04
Copy link
Member

@marqdevx marqdevx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I have some doubts/suggestions, please if I'm wrong reply to the comment with the reason

Thanks!! 😄

- **Frequency = SampleRate x BitsPerChannel x numberOfChannels**

In a typical setup, the sender of audio data is called a Transmitter and it transfers data to a Receiver at the other end. The device that controls the bus clock, SCK, together with the Word Select - WS - signal is the Controller in the network and in any network just one device can be Controller at any given time; all the other devices connected are in Peripheral mode. The Controller can be the Transmitter, or the Receiver, or a standalone controller. The digitized audio data sample can have a size ranging from 4 bits up to 32.
The left and right channels are interleaved in the data stream, and "one sample" is one value for the left channel and one for the right. This means that the FS line oscillates at the sample rate.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not 100% needed, could you elaborate this paragraph more maybe?

Copy link
Author

@stegu stegu Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment about the FS line is valid only for 2 channels, so scratch that. The rest is exposition about the I2S protocol, and that can also be scrapped, although documentation on the I2S protocol is generally hard to find and difficult to read. This is generic documentation, not a more narrowly focused "howto", and I like to educate people. (Work related habit, do as you like.)


#### Description
Get the number of bytes available for reading from the I2S interface. This is data that has already arrived and was stored in the I2S receive buffer. available() inherits from the Stream utility class.
Get the number of **bytes** (not the number of samples) available for reading from the I2S interface. This is data that has already arrived and was stored in the I2S receive buffer. available() inherits from the Stream utility class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the contrary, my experience is that the most common trap people fall into is to confuse the number of bytes with the number of samples for these functions. This is based on a study of only two test subjects, me and my colleague, but I think it is motivated to point this out. Do as you like.

Comment on lines +120 to +123
The single argument function `write(val)` writes one sample for one channel. A full write requires
calling this function twice, once for the left channel and once for the right channel.
If the device buffer is full, the function blocks until the data can be written.
Before writing, the value `val` will be converted to the appropriate number of bits per sample.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be n times depending on the amount of channels, if you are only working with one channel then its only once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I have never worked with mono or multi-channel devices.

Comment on lines +125 to +131
The two-argument function `write(buf, len)` writes a sequence of samples from the array `buf`.
Data must be stored in the proper format for the sample size, with the left and right channels
interleaved, and the argument `len` specifies the number of **bytes** in the buffer, not the
number of samples. If the device buffer is too full to hold all the data, the function writes
as much as will fit (possibly nothing), and returns without blocking. The return value is the
number of bytes that was actually written. It is the responsibility of the application to
handle any such partial writes and act accordingly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be more generic, always referring to the 2 channel scenario

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. my bad. See above.

val: a value to send as a single sample

buf: an array to send as a series of samples
buf: an array to send as a sequence of samples
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In digital signal processing, the correct term is "sequence" of samples. In mathematics, a "series" is something different. There is no real risk of confusion here, but "sequence" is the term I would strongly recommend.

Comment on lines +207 to +208
val = I2S.read() // Might block until data is available for reading
I2S.read(buf, len) // Will not block, but might not read all the data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
val = I2S.read() // Might block until data is available for reading
I2S.read(buf, len) // Will not block, but might not read all the data
int val = I2S.read(); // Might block until data is available for reading
int bufferVal = I2S.read(buf, len); // Will not block, but might not read all the data

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. Sorry. I'm unfamiliar with the format for this documentation.


buf: an array to store a sequence of samples

len: the length of the `buf` array, in bytes (not in number of samples)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the content of the parenthesis

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not mentioned here, definitely deserves pointing out elsewhere. Common pitfall.


buf: an array to store a sequence of samples

len: the length of the `buf` array, in bytes (not in number of samples)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
len: the length of the `buf` array, in bytes (not in number of samples)
len: the length of the `buf` array, in bytes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

len: the length of the `buf` array, in bytes (not in number of samples)

#### Returns
size_t
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does return an int, isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the source, it actually does. It should return a size_t, reasonably, but it doesn't.


#### Description
Registers a function to be called when a block of data has been received.
Registers a function to be called when a block of data has been received. This works just like the `onTransmit()` handler, but for the `read(buf, len)` function that receives data from a device. The callback is not triggered for the no-argument version of `read()` that reads a single sample value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned before send us the source of this please! 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was verified by my own experiments. I don't see it mentioned anywhere in the docs, and this was a major headache for me. The fact that the loop() function executes at the sample rate would also be worth mentioning. It's not explicitly stated anywhere in the current docs.

@marqdevx
Copy link
Member

marqdevx commented Jun 29, 2022

Thanks for the replies, I will check them out. 😄

Also maybe you should think on creating a tutorial based on your project if you want.

This article is more a library reference than a tutorial, it was taken from old documentation. :)

@stegu stegu requested a review from marqdevx June 29, 2022 09:40
@stegu
Copy link
Author

stegu commented Jun 29, 2022

I will certainly consider writing a "proper" tutorial, although I have a very limited selection of hardware available to me: just the one stereo amplifier and one Arduino board with an on-board microphone. This was just a quick fix for other people to hopefully be able to dodge some of the pitfalls I encountered.

@marqdevx
Copy link
Member

Sounds good!
Which board are you using btw? 🤔
I might try it out

@stegu
Copy link
Author

stegu commented Jun 29, 2022 via email

@marqdevx marqdevx self-assigned this Aug 23, 2022
@marqdevx
Copy link
Member

Closing the PR due to no changes

Please feel free to re-open it to do the changes

@marqdevx marqdevx closed this Dec 15, 2022
@stegu
Copy link
Author

stegu commented Dec 15, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Bugs and fixes suggested by the community fix/update A small fix or update Tutorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants