Review #2
Review #2
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
-NOTE_ constants all updated to kSfeQwiicBuzzerB0, etc -register file name updated -pitches file name updated
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
-this is a more efficient way to write all the config registers in one single I2C write -increased I2C clock for sound effect example. This ensures all the rapid config changes during some of the faster effects.
This comment was marked as resolved.
This comment was marked as resolved.
sound effects don't return anything
-not needed, because of the delays
This comment was marked as resolved.
This comment was marked as resolved.
-on() no longer takes settings arguments. It soley sets the buzzerActiveRegister to turn on the buzzer -configureBuzzer must be called separately in examples to configure the buzzer -removed silentConfiguration example 9 - because this is done in most other examples already. That is, when you call configureBuzzer(), this does not cause the buzzer to buzz, and so is "silent".
This comment was marked as resolved.
This comment was marked as resolved.
-the header and cpp files are generic c++ files and do not have any "arduino" code in them.
This comment was marked as resolved.
This comment was marked as resolved.
gigapod
left a comment
gigapod
left a comment
There was a problem hiding this comment.
A couple of minor things to cleanup here.
| void setup() { | ||
| Serial.begin(115200); | ||
| Serial.println("Qwiic Buzzer Example_09_FirmwareVersion"); | ||
| Wire.begin(); //Join I2C bus |
There was a problem hiding this comment.
Is this needed? If no wire is provided to begin - which these calls into the toolkit which uses the default Wire, it calls begin I believe.
The correct action is one of the following:
- Remove Wire.begin()
- Keep Wire.begin(), and then pass that into the begin() method - you are taking ownership for wire setup ..etc
examples/Example_09_FirmwareVersion/Example_09_FirmwareVersion.ino
Outdated
Show resolved
Hide resolved
src/sfeQwiicBuzzerPitches.h
Outdated
| Public Constants | ||
| *************************************************/ | ||
|
|
||
| #define SFE_QWIIC_BUZZER_NOTE_B0 31 |
There was a problem hiding this comment.
nit note: These should be const uint*_t values.
In c++, you should rarely use #define constant defines.
| @@ -0,0 +1,41 @@ | |||
| /****************************************************************************** | |||
There was a problem hiding this comment.
This is fine and I won't push this. But future implementations, we need to change how this is done.
I could be two registers, but with active developed products it could/will cause an issue at some point. Also it's important to have a mindset of defensive programming when implementing functionally. Prevents future issues and very little cost up front.
ready for group code review.