Open
Conversation
Reviewer's GuideThis PR adds Arduino Micro (AVR/ATmega32U4) compatibility by moving note tables into PROGMEM, switching from SAMD FlashStorage to AVR EEPROM, simplifying the buzzer to a single tone slot, disabling capacitive touch for this target, and introducing an Arduino-Micro-specific config with adjusted pin/LED behavior and timing types. Sequence diagram for saving settings to EEPROM on Arduino MicrosequenceDiagram
actor User
participant VailAdapter
participant SettingsEEPROM as settings_eeprom
participant EEPROMLib as EEPROM
User->>VailAdapter: Change configuration (keyer type, dit duration, tx note)
VailAdapter->>SettingsEEPROM: saveSettingsToEEPROM(keyerType, ditDuration, txNote)
SettingsEEPROM->>EEPROMLib: EEPROM.write(EEPROM_KEYER_TYPE_ADDR, keyerType)
SettingsEEPROM->>EEPROMLib: EEPROM.put(EEPROM_DIT_DURATION_ADDR, ditDuration)
SettingsEEPROM->>EEPROMLib: EEPROM.write(EEPROM_TX_NOTE_ADDR, txNote)
SettingsEEPROM->>EEPROMLib: EEPROM.write(EEPROM_VALID_FLAG_ADDR, EEPROM_VALID_VALUE)
Note over SettingsEEPROM,EEPROMLib: No EEPROM.commit call on AVR
SettingsEEPROM-->>VailAdapter: return
VailAdapter-->>User: Settings persisted across reset
Class diagram for updated PolyBuzzer and keyer timingclassDiagram
class PolyBuzzer {
+unsigned int tones[POLYBUZZER_MAX_TONES]
+unsigned int playing
+uint8_t pin
+PolyBuzzer(uint8_t pin)
+void update()
+void Tone(int slot, unsigned int frequency)
+void Note(int slot, uint8_t note)
+void NoTone(int slot)
}
class TouchBounce {
+void attach(int pin, int pressThreshold, int releaseThreshold)
+bool readCurrentState()
+Adafruit_FreeTouch qt
+bool lastState
+int pressThreshold
+int releaseThreshold
}
class Bounce {
}
class Transmitter {
+virtual void BeginTx()
+virtual void EndTx()
+virtual void BeginTx(int relay)
+virtual void EndTx(int relay)
}
class Keyer {
+Transmitter* output
+virtual void Tx(int relay, bool closed)
+virtual void Key(Paddle key, bool pressed)
+virtual void Tick(unsigned long millis)
}
class StraightKeyer {
+virtual void Key(Paddle key, bool pressed)
+virtual void Tick(unsigned long millis)
}
class BugKeyer {
+unsigned long nextPulse
+bool keyPressed[2]
+virtual void Key(Paddle key, bool pressed)
+virtual void Tick(unsigned long millis)
+virtual void pulse(unsigned long millis)
}
class ElBugKeyer {
+virtual void pulse(unsigned long millis)
}
class VailAdapter {
+void Tick(unsigned long currentMillis)
+void DisableBuzzer()
+bool ditIsHeld
+unsigned long ditHoldStartTime
+bool buzzerEnabled
}
Bounce <|-- TouchBounce
Transmitter <|-- VailAdapter
Keyer <|-- StraightKeyer
StraightKeyer <|-- BugKeyer
BugKeyer <|-- ElBugKeyer
VailAdapter --> PolyBuzzer
class EqualTemperamentTable {
+static const uint16_t equalTemperamentNote[128]
+uint16_t GET_EQUAL_TEMPERAMENT_NOTE(uint8_t n)
}
Class diagram for TouchBounce conditional compilation and thresholdsclassDiagram
class TouchBounce {
+void attach(int pin, int pressThreshold, int releaseThreshold)
+bool readCurrentState()
+Adafruit_FreeTouch qt
+bool lastState
+int pressThreshold
+int releaseThreshold
}
class Bounce {
}
class ConfigMacros {
+#define NO_CAPACITIVE_TOUCH
+#define QT_DIT_THRESHOLD_PRESS 450
+#define QT_DIT_THRESHOLD_RELEASE 360
+#define QT_DAH_THRESHOLD_PRESS 450
+#define QT_DAH_THRESHOLD_RELEASE 360
}
Bounce <|-- TouchBounce
ConfigMacros ..> TouchBounce
Class diagram for EEPROM-based settings storageclassDiagram
class VailAdapter {
+uint8_t getKeyerType()
+uint16_t getDitDuration()
+uint8_t getTxNote()
+bool getRadioKeyerMode()
}
class SettingsEEPROM {
+void saveSettingsToEEPROM(uint8_t keyerType, uint16_t ditDuration, uint8_t txNote)
+void saveRadioKeyerModeToEEPROM(bool radioKeyerMode)
+void loadSettingsFromEEPROM(VailAdapter& adapter)
+void saveMemoryToEEPROM(uint8_t slotNumber, const CWMemory& memory)
+void clearMemoryInEEPROM(uint8_t slotNumber)
}
class EEPROMLib {
+uint8_t read(int address)
+void write(int address, uint8_t value)
+template<typename T> void put(int address, const T& value)
}
class CWMemory {
+uint16_t transitionCount
+uint16_t transitions[64]
}
VailAdapter --> SettingsEEPROM
SettingsEEPROM --> EEPROMLib
SettingsEEPROM --> CWMemory
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The change to
POLYBUZZER_MAX_TONES= 1 and ignoring slots other than 0 inPolyBuzzerapplies globally; if you still want polyphonic/prioritized tones on non-AVR targets, consider wrapping the single-slot behavior in an#ifdef(e.g.,#ifdef ARDUINO_AVR_MICRO) rather than changing the class semantics for all boards. - In
config.hthe previous board-specific configurations have been replaced by an Arduino Micro–only setup; if you intend to keep supporting the earlier QT Py / PCB variants, it might be cleaner to introduce a dedicatedARDUINO_MICROconfig branch instead of removing the existing ones outright. - The call to
DisableBuzzer()inVailAdapter::Tickis now commented out, which changes long-press behavior on all platforms; consider using a compile-time flag (e.g.,#ifdef ARDUINO_AVR_MICRO) or a runtime setting so memory-constrained boards can opt out without altering behavior for other targets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change to `POLYBUZZER_MAX_TONES` = 1 and ignoring slots other than 0 in `PolyBuzzer` applies globally; if you still want polyphonic/prioritized tones on non-AVR targets, consider wrapping the single-slot behavior in an `#ifdef` (e.g., `#ifdef ARDUINO_AVR_MICRO`) rather than changing the class semantics for all boards.
- In `config.h` the previous board-specific configurations have been replaced by an Arduino Micro–only setup; if you intend to keep supporting the earlier QT Py / PCB variants, it might be cleaner to introduce a dedicated `ARDUINO_MICRO` config branch instead of removing the existing ones outright.
- The call to `DisableBuzzer()` in `VailAdapter::Tick` is now commented out, which changes long-press behavior on all platforms; consider using a compile-time flag (e.g., `#ifdef ARDUINO_AVR_MICRO`) or a runtime setting so memory-constrained boards can opt out without altering behavior for other targets.
## Individual Comments
### Comment 1
<location path="equal_temperament.h" line_range="5" />
<code_context>
- 11181, // 125
- 11845, // 126
- 12550, // 127
+static const uint16_t equalTemperamentNote[] PROGMEM = {
+ 8,8,9,9,10,10,11,12,12,13,14,15,16,17,18,19,
+ 20,21,23,24,25,27,29,30,32,34,36,38,41,43,46,49,
</code_context>
<issue_to_address>
**suggestion (performance):** Using `static` in the header will duplicate the lookup table into every translation unit
Declaring this table as `static const` in the header gives each including `.cpp` its own flash/PROGMEM copy, which is wasteful on small AVR devices. Instead, declare it `extern` in the header and define it once in a `.cpp`:
```c++
// equal_temperament.h
extern const uint16_t equalTemperamentNote[] PROGMEM;
// equal_temperament.cpp
#include <Arduino.h>
#include <avr/pgmspace.h>
const uint16_t equalTemperamentNote[] PROGMEM = { ... };
```
`GET_EQUAL_TEMPERAMENT_NOTE` will still work unchanged with the `extern` declaration.
Suggested implementation:
```c
extern const uint16_t equalTemperamentNote[] PROGMEM;
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])
```
` since that file isn't shown).
Here are the edits to `equal_temperament.h`:
```xml
<file_operations>
<file_operation operation="edit" file_path="equal_temperament.h">
<<<<<<< SEARCH
static const uint16_t equalTemperamentNote[] PROGMEM = {
8,8,9,9,10,10,11,12,12,13,14,15,16,17,18,19,
20,21,23,24,25,27,29,30,32,34,36,38,41,43,46,49,
51,55,58,61,65,69,73,77,82,87,92,98,103,110,116,123,
130,138,146,155,164,174,185,196,207,220,233,247,261,277,293,311,
329,349,370,392,415,440,466,494,523,554,587,622,659,698,740,784,
831,880,932,988,1047,1109,1175,1245,1319,1397,1480,1568,1662,1760,1865,1976,
2094,2218,2350,2490,2638,2795,2961,3137,3324,3521,3731,3953,4188,4437,4701,4980,
5276,5590,5922,6275,6648,7043,7462,7906,8376,8874,9402,9961,10553,11181,11845,12550
};
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])
=======
extern const uint16_t equalTemperamentNote[] PROGMEM;
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])
>>>>>>> REPLACE
</file_operation>
</file_operations>
```
<additional_changes>
1. Add a new implementation file `equal_temperament.cpp` with the single definition of the lookup table:
```c++
#include <Arduino.h>
#include <avr/pgmspace.h>
#include "equal_temperament.h"
const uint16_t equalTemperamentNote[] PROGMEM = {
8,8,9,9,10,10,11,12,12,13,14,15,16,17,18,19,
20,21,23,24,25,27,29,30,32,34,36,38,41,43,46,49,
51,55,58,61,65,69,73,77,82,87,92,98,103,110,116,123,
130,138,146,155,164,174,185,196,207,220,233,247,261,277,293,311,
329,349,370,392,415,440,466,494,523,554,587,622,659,698,740,784,
831,880,932,988,1047,1109,1175,1245,1319,1397,1480,1568,1662,1760,1865,1976,
2094,2218,2350,2490,2638,2795,2961,3137,3324,3521,3731,3953,4188,4437,4701,4980,
5276,5590,5922,6275,6648,7043,7462,7906,8376,8874,9402,9961,10553,11181,11845,12550
};
```
2. Ensure `equal_temperament.cpp` is compiled/linked as part of the project so the `equalTemperamentNote` symbol is available to all translation units that include `equal_temperament.h`.
</issue_to_address>
### Comment 2
<location path="polybuzzer.cpp" line_range="31" />
<code_context>
-
- this->tones[slot] = frequency;
- this->update();
+ if (slot != 0) return; // ignore extra slots on Micro
+
+ Serial.print(F("Setting tone in slot "));
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silently ignoring `slot != 0` can hide logic bugs at call sites
Calls with `slot != 0` effectively do nothing while still appearing to succeed, which can be confusing for callers that expect multiple slots. Consider either clamping `slot` to 0 or adding a debug-only warning (and then clamping) so that misuse is visible during development instead of silently dropping tones.
Suggested implementation:
```cpp
void PolyBuzzer::Tone(int slot, unsigned int frequency) {
int clampedSlot = slot;
if (slot != 0) {
#ifdef DEBUG
Serial.print(F("PolyBuzzer::Tone: requested slot "));
Serial.print(slot);
Serial.println(F(" not supported on Micro; clamping to slot 0"));
#endif
clampedSlot = 0;
}
Serial.print(F("Setting tone in slot "));
Serial.print(clampedSlot);
Serial.print(F(" to frequency: "));
Serial.println(frequency);
this->tones[clampedSlot] = frequency;
this->update();
}
```
1. Ensure that `DEBUG` (or whatever debug macro is appropriate for this project) is defined in your build system or a common header to enable the warning during development.
2. If the project uses a different debug macro (e.g. `DEBUG_POLYBUZZER`, `SERIAL_DEBUG`, etc.), replace `#ifdef DEBUG` with the project-standard macro for conditional debug logging.
</issue_to_address>
### Comment 3
<location path="adapter.cpp" line_range="835" />
<code_context>
Serial.print("Dit held for ");
Serial.print(holdTime);
Serial.println("ms - disabling buzzer");
- this->DisableBuzzer();
+ //this->DisableBuzzer();
this->ditIsHeld = false; // Reset to prevent re-triggering
} else if (holdTime % 1000 == 0) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Disabling the buzzer auto-shutoff logic may reintroduce long-hold edge cases
With `DisableBuzzer()` commented out, `ditIsHeld` is cleared but the buzzer stays on, so any logic that relied on long-hold auto-shutoff may now see an indefinite tone.
If this behavioral change is intentional for the Micro, consider either:
- Removing this long-hold block entirely (to avoid misleading "disabling buzzer" logs), or
- Guarding the auto-shutoff with a board-specific flag (e.g. `#ifdef DISABLE_BUZZER_AUTOSHUTOFF`) so the change is explicit and configuration-driven.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 11181, // 125 | ||
| 11845, // 126 | ||
| 12550, // 127 | ||
| static const uint16_t equalTemperamentNote[] PROGMEM = { |
There was a problem hiding this comment.
suggestion (performance): Using static in the header will duplicate the lookup table into every translation unit
Declaring this table as static const in the header gives each including .cpp its own flash/PROGMEM copy, which is wasteful on small AVR devices. Instead, declare it extern in the header and define it once in a .cpp:
// equal_temperament.h
extern const uint16_t equalTemperamentNote[] PROGMEM;
// equal_temperament.cpp
#include <Arduino.h>
#include <avr/pgmspace.h>
const uint16_t equalTemperamentNote[] PROGMEM = { ... };GET_EQUAL_TEMPERAMENT_NOTE will still work unchanged with the extern declaration.
Suggested implementation:
extern const uint16_t equalTemperamentNote[] PROGMEM;
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])` since that file isn't shown).
Here are the edits to equal_temperament.h:
<file_operations>
<file_operation operation="edit" file_path="equal_temperament.h">
<<<<<<< SEARCH
static const uint16_t equalTemperamentNote[] PROGMEM = {
8,8,9,9,10,10,11,12,12,13,14,15,16,17,18,19,
20,21,23,24,25,27,29,30,32,34,36,38,41,43,46,49,
51,55,58,61,65,69,73,77,82,87,92,98,103,110,116,123,
130,138,146,155,164,174,185,196,207,220,233,247,261,277,293,311,
329,349,370,392,415,440,466,494,523,554,587,622,659,698,740,784,
831,880,932,988,1047,1109,1175,1245,1319,1397,1480,1568,1662,1760,1865,1976,
2094,2218,2350,2490,2638,2795,2961,3137,3324,3521,3731,3953,4188,4437,4701,4980,
5276,5590,5922,6275,6648,7043,7462,7906,8376,8874,9402,9961,10553,11181,11845,12550
};
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])
=======
extern const uint16_t equalTemperamentNote[] PROGMEM;
#define GET_EQUAL_TEMPERAMENT_NOTE(n) pgm_read_word(&equalTemperamentNote[(n)])
>>>>>>> REPLACE
</file_operation>
</file_operations><additional_changes>
- Add a new implementation file
equal_temperament.cppwith the single definition of the lookup table:
#include <Arduino.h>
#include <avr/pgmspace.h>
#include "equal_temperament.h"
const uint16_t equalTemperamentNote[] PROGMEM = {
8,8,9,9,10,10,11,12,12,13,14,15,16,17,18,19,
20,21,23,24,25,27,29,30,32,34,36,38,41,43,46,49,
51,55,58,61,65,69,73,77,82,87,92,98,103,110,116,123,
130,138,146,155,164,174,185,196,207,220,233,247,261,277,293,311,
329,349,370,392,415,440,466,494,523,554,587,622,659,698,740,784,
831,880,932,988,1047,1109,1175,1245,1319,1397,1480,1568,1662,1760,1865,1976,
2094,2218,2350,2490,2638,2795,2961,3137,3324,3521,3731,3953,4188,4437,4701,4980,
5276,5590,5922,6275,6648,7043,7462,7906,8376,8874,9402,9961,10553,11181,11845,12550
};- Ensure
equal_temperament.cppis compiled/linked as part of the project so theequalTemperamentNotesymbol is available to all translation units that includeequal_temperament.h.
|
|
||
| this->tones[slot] = frequency; | ||
| this->update(); | ||
| if (slot != 0) return; // ignore extra slots on Micro |
There was a problem hiding this comment.
suggestion (bug_risk): Silently ignoring slot != 0 can hide logic bugs at call sites
Calls with slot != 0 effectively do nothing while still appearing to succeed, which can be confusing for callers that expect multiple slots. Consider either clamping slot to 0 or adding a debug-only warning (and then clamping) so that misuse is visible during development instead of silently dropping tones.
Suggested implementation:
void PolyBuzzer::Tone(int slot, unsigned int frequency) {
int clampedSlot = slot;
if (slot != 0) {
#ifdef DEBUG
Serial.print(F("PolyBuzzer::Tone: requested slot "));
Serial.print(slot);
Serial.println(F(" not supported on Micro; clamping to slot 0"));
#endif
clampedSlot = 0;
}
Serial.print(F("Setting tone in slot "));
Serial.print(clampedSlot);
Serial.print(F(" to frequency: "));
Serial.println(frequency);
this->tones[clampedSlot] = frequency;
this->update();
}
- Ensure that
DEBUG(or whatever debug macro is appropriate for this project) is defined in your build system or a common header to enable the warning during development. - If the project uses a different debug macro (e.g.
DEBUG_POLYBUZZER,SERIAL_DEBUG, etc.), replace#ifdef DEBUGwith the project-standard macro for conditional debug logging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds Arduino Micro compatibility changes for the Vail Adapter sketch.
Changes include:
Notes:
Summary by Sourcery
Add Arduino Micro–specific configuration and memory optimizations, replacing SAMD-only features and adapting audio, touch, and keyer timing to work on AVR/ATmega32U4.
New Features:
Bug Fixes:
Enhancements: