Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sound meter code to work with CPB and CPX #93

Closed
wants to merge 3 commits into from

Conversation

kattni
Copy link
Contributor

@kattni kattni commented Apr 16, 2020

Fixes #76

@kattni kattni requested a review from a team April 16, 2020 20:39
)
# Only Circuit Playground Express has touch_A7 available. So, we'll use it to determine whether or
# not the board being used is a CPX. If it is a CPX, run the first code block.
if hasattr(cp, "touch_A7"):
Copy link
Member

Choose a reason for hiding this comment

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

add a test to the cp object so you can ask which board you got :)

@kattni kattni requested a review from a team April 29, 2020 19:53
@@ -78,6 +79,9 @@ class CircuitPlaygroundBase: # pylint: disable=too-many-public-methods

_audio_out = None

# Circuit Playground specific board names as found in os.uname().machine
_CP_TYPES = ("Bluefruit", "Express")
Copy link
Member

Choose a reason for hiding this comment

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

@dhalbert would you think doing this consts make more sense?

Copy link
Contributor

@dhalbert dhalbert May 1, 2020

Choose a reason for hiding this comment

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

I suggested consts as a possibility but @kattni preferred strings. We need the strings anyway to search for them in the os.uname() field to distinguish the boards.

We could use strings that have class attribute names too.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Why aren't you just implementing sound_level on the Express class? That would remove the need for conditional examples at all.

All other comments are based on doing conditional examples but it gets hairy pretty quickly.


This can be simplified by using Python's built in type logic. It has isinstance() which can be used to see if the CP object is a Bluefruit or Express object. It doesn't require any changes to the base.

init.py will need to be:

if sys.platform == "nRF52840":
    import bluefruit
    cp = bluefruit.Bluefruit()
elif sys.platform == "Atmel SAMD21":
    import express
    cp = express.Express()

The constructors will need to be moved... which means a bunch of guides will need to be updated.

https://gist.github.com/tannewt/69bee4b4ebe8e13bba9b256943474bb0

board.MICROPHONE_CLOCK, board.MICROPHONE_DATA, sample_rate=16000, bit_depth=16
)
# Check to see if the board type is a Circuit Playground Express, and, if so, run the following:
if cp.circuit_playground_is_type("Express"):
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
if cp.circuit_playground_is_type("Express"):
if isinstance(cp, Express):

Copy link
Member

Choose a reason for hiding this comment

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

This will also need another import from adafruit_circuitplayground.express import Express and the `cp instantiate will need to be in init (otherwise this import will construct the cpx object.)

@kattni
Copy link
Contributor Author

kattni commented Apr 30, 2020

@tannewt We designed this to be backwards compatible because there is SO much usage of this code using the previous from adafruit_circuitplayground import cpx code. If I understand you correctly, you're suggesting removing backwards compatibility?

sound_level is not implemented in Express because it doesn't fit. We've discussed this multiple times. It uses a buffer to get sound samples and average them over time to get a sound level. You can implement it on Express, and use only it in code.py and it works, but as soon as you add any other code, memory allocation fails. The M0 cannot handle it, that is why it was never implemented in the original library before Bluefruit was added.

@tannewt
Copy link
Member

tannewt commented May 1, 2020

@tannewt We designed this to be backwards compatible because there is SO much usage of this code using the previous from adafruit_circuitplayground import cpx code. If I understand you correctly, you're suggesting removing backwards compatibility?

Originally I was but I think implementing sound_level is much simpler.

sound_level is not implemented in Express because it doesn't fit. We've discussed this multiple times. It uses a buffer to get sound samples and average them over time to get a sound level. You can implement it on Express, and use only it in code.py and it works, but as soon as you add any other code, memory allocation fails. The M0 cannot handle it, that is why it was never implemented in the original library before Bluefruit was added.

When you say "don't fit" do you mean in memory or in flash when frozen? Is there an existing implementation that doesn't work? I can help get it to fit. I'm surprised that the type functions in this PR are any less space than implementing sound_level.

I don't want to see a non-Python way of checking the type of something. There are already ways to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sound_meter example fails on Circuit Playground Bluefruit
4 participants