adafruit / Adafruit_CircuitPython_CircuitPlayground

CircuitPython library for Circuit Playground Express
MIT License
91 stars 71 forks source link

Added square wave functionality to play tone. #116

Closed ryanskeith closed 2 years ago

ryanskeith commented 2 years ago

Added code to enable square tone. This turns out to be much louder. In this addition, the variable names that referred to sine waves were generalized to _wave and _wave sample. This is a proposed solution for issue #49. And the square wave is much louder.

FoamyGuy commented 2 years ago

The actions check failed due to code formatting. There is a guide page here that has more information and shows how to get set up locally to run the check and have it do the formatting changes for you. https://learn.adafruit.com/creating-and-sharing-a-circuitpython-library/check-your-code

We'll also need to double check whether there is enough room on the CPX builds to include this change. iirc it's a rather tight fit and this library is frozen in so increases in code size here can possibly make it not fit within that build.

ryanskeith commented 2 years ago

I feel this is completed and ready for review.

kattni commented 2 years ago

Unfortunately, I don't have a CPX with me to test this with. I can test the build sizes though.

tekktrik commented 2 years ago

Linked the relevant issue since it seems to address it.

FoamyGuy commented 2 years ago

I successfully tested the functionality with a custom build of CPX with this branch as the frozen in lib. It is a good margin louder.

I tried one of the builds that I knew from a prior PR was most limited in space and unfortunately this does push it over the limit:

make BOARD=circuitplayground_express_displayio TRANSLATION=ru
...
253564 bytes used, -124 bytes free in flash firmware space out of 253440 bytes (247.5kB).
12192 bytes used, 20576 bytes free in ram for stack and heap out of 32768 bytes (32.0kB).

Too little flash!!!
kattni commented 2 years ago

@FoamyGuy I completely forgot about the displayio build. I tested the regular build for CPX and it's fine.

@dhalbert Is there any way to shrink the displayio build? Or is that not feasible here?

ryanskeith commented 2 years ago

So close, yet so far away... This issue may need to be dropped as I am not seeing a way to reduce how to simplify the solution further. Alternatively, you could adopt to use the square wave versus sine as it is louder.

FoamyGuy commented 2 years ago

That thought of changing to use only the square wave function and remove sine crossed my mind as a possibility too. I don't necessarily have a super strong preference either way but the louder one is easier to tell when it's working depending on the frequencies used.

kattni commented 2 years ago

Let's also give Dan a chance to let us know if he can do something with the displayio build. If not, then we'll make a decision on how to move forward here.

dhalbert commented 2 years ago

Setting CIRCUITPY_ONEWIREIO = 0 in the build would recover 500-600 bytes.

dhalbert commented 2 years ago

So if you go ahead and submit this, then when we update the frozen modules the next time, I will also shrink that build.

kattni commented 2 years ago

Excellent, thank you @dhalbert!

@FoamyGuy Go ahead and review it with that in mind, thanks!

FoamyGuy commented 2 years ago

I tried the same build with CIRCUITPY_ONEWIREIO = 0

But it still came out just a tad over:

253476 bytes used, -36 bytes free in flash firmware space out of 253440 bytes (247.5kB).
12192 bytes used, 20576 bytes free in ram for stack and heap out of 32768 bytes (32.0kB).

This is with the branch from this PR and CIRCUITPY_ONEWIREIO = 0 on CPX_Displayio with ru translation.

kattni commented 2 years ago

Oof, ok. @FoamyGuy let's still review this with the assumption that we can make it fit and then make a decision. Thanks!

dhalbert commented 2 years ago

Ugh, well, I think I can find something or other.

FoamyGuy commented 2 years ago

fwiw I did also try with deleting bluefruit.py inside the frozen library and it does fit with a good amount of head-room like that:

252304 bytes used, 1136 bytes free in flash firmware space out of 253440 bytes (247.5kB).
12192 bytes used, 20576 bytes free in ram for stack and heap out of 32768 bytes (32.0kB).

(same build for CPX_Displayio ru and this branch of the library).

If we can come up with a mechanism that allows excluding files from there it could give us enough space I think.

ryanskeith commented 2 years ago

I moved from string to number way of doing things to determine SINE or SQUARE. Let me know if it made the space cut @FoamyGuy, @kattni .

FoamyGuy commented 2 years ago

Tested that same cpx displayio+ru build on the latest version and still not quite fitting -56 bytes on the latest one.

We'll look into potential refactors to squeeze it a bit more and then eventually maybe try to have some mechanism that can exclude files that are unneeded to gain back some space.

tekktrik commented 2 years ago

Another potential fix is #117 by @neradoc

kattni commented 2 years ago

Merging this with the understanding that we will need to tweak the build when we update the frozen modules.

@ryanskeith Thank you again!