Support for hardware versions. Added NeoPixel. #175 #181
Conversation
microcontroller-src/kv4p_ht_esp32_wroom_32/kv4p_ht_esp32_wroom_32.ino
Outdated
Show resolved
Hide resolved
|
Please merge with main, resolve conflicts (mainly around how firmware determines VHF/UHF), and test. When I did that, I get this error (seen in serial output from the ESP32): Guru Meditation Error: Core 1 panic'ed (LoadProhibited). Exception was unhandled. It then crashes and restarts the firmware, this happens on a loop. |
Done.
Are you seeing this when just connected to a terminal emulator? Or is there a way to see what's coming over serial in the app? |
|
What do you see on the phone app when this happens? I've got it saying "No firmware installed." Is that what you're seeing? I'm just trying to confirm whether I'm chasing a phantom or not. |
|
Yes, it says no firmware installed (because the ESP32 isn't replying to the GET_VERSION command). To further investigate I reviewed the Arduino IDE terminal with the ESP32 connected and see the error I included above. I've never seen that error before, so I suspect it's something in this PR. |
| @@ -730,3 +832,52 @@ void processTxAudio(uint8_t tempBuffer[], int bytesRead) { | |||
| bytesToWrite -= bytesWritten; | |||
| } while (bytesToWrite > 0); | |||
| } | |||
|
|
|||
| uint8_t get_hardware_version() { | |||
There was a problem hiding this comment.
This seems too complicated to what it does. Is it effectively the same to the:
uint8_t get_hardware_version() {
pinMode(HW_VER_0_PIN, INPUT_PULLDOWN); pinMode(HW_VER_1_PIN, INPUT_PULLDOWN);
uint8_t down = (digitalRead(HW_VER_1_PIN) << 1) | digitalRead(HW_VER_0_PIN);
pinMode(HW_VER_0_PIN, INPUT_PULLUP); pinMode(HW_VER_1_PIN, INPUT_PULLUP);
uint8_t up = (digitalRead(HW_VER_1_PIN) << 1) | digitalRead(HW_VER_0_PIN);
return (down == up) ? down : 0xFF; // Return version or error
}
And if there no pull-ups support on those pins, we can do:
uint8_t get_hardware_version() {
pinMode(HW_VER_0_PIN, INPUT_PULLDOWN); pinMode(HW_VER_1_PIN, INPUT_PULLDOWN);
return (digitalRead(HW_VER_1_PIN) << 1) | digitalRead(HW_VER_0_PIN);
}
There was a problem hiding this comment.
I realized the "no pull-[up|down]" thing as I was filing the PR, after already making all the changes. You're right that this can be WAY simpler now. I'm just kicking myself for not looking at this before releasing v2.0c into the wild. sigh
|
I've confirmed I'm fighting the fact that I don't have the most current app version on my phone. That means I have to setup the Google app development environment, which I don't have the time to do today. I've already spent longer on this PR than I intended today. On the plus side, all my new code: NeoPixels, Hardware Versions, and the new SoftwareSerial console I've put on the GPIO header, are all Yeah, I just confirmed that comms are stuck at getting a firmware version. I can't debug anymore without the current app version. This will have to be later. EDIT: Oh, look, that's what you just said 13 minutes ago: #181 (comment) Ok, so that is the same thing you're seeing. I'll dig a little more, but I didn't touch the serial protocol handling. I did notice that at the top of loop(), you're just counting 8 characters without actually testing that they're the delimiter characters. This is likely to get things out of sync. So I added a couple lines that reset |
| @@ -524,18 +613,26 @@ void loop() { | |||
| size_t samplesRead = bytesRead / 2; | |||
|
|
|||
| bool squelched = (digitalRead(SQ_PIN) == HIGH); | |||
| if (first_time) { | |||
There was a problem hiding this comment.
it’s probably a good idea to extract the LED control and pixel operations into a separate function like displayUserFeedback(). Something like:
enum Mode {
MODE_STOPPED,
MODE_RX_OPEN,
MODE_RX_CLOSED,
MODE_TX
};
void displayUserFeedback(Mode mode) {
switch (mode) {
case MODE_STOPPED:
digitalWrite(LED_PIN, LOW);
digitalWrite(PTT_PIN, HIGH);
pixels.setPixelColor(0, COLOR_STOPPED);
break;
case MODE_RX_OPEN:
digitalWrite(LED_PIN, LOW);
pixels.setPixelColor(0, COLOR_RX_SQL_OPEN);
break;
case MODE_RX_CLOSED:
digitalWrite(LED_PIN, LOW);
pixels.setPixelColor(0, COLOR_RX_SQL_CLOSED);
break;
case MODE_TX:
digitalWrite(LED_PIN, HIGH); // LED ON for TX
pixels.setPixelColor(0, COLOR_TX);
break;
default:
break;
}
pixels.show();
}
There was a problem hiding this comment.
Agreed. After doing all the #ifdef stuff, I was very quickly regretting doing it all inline and not abstracting it out. Now that I'm re-doing it all, I will do it better.
There was a problem hiding this comment.
This is done in #207
| // Setup NeoPixel for debugging. | ||
| #define PIXELS_NUM 1 | ||
| #define PIXELS_PIN 13 | ||
| Adafruit_NeoPixel pixels(PIXELS_NUM, PIXELS_PIN, NEO_GRB + NEO_KHZ400); |
There was a problem hiding this comment.
Is it possible to unit and call NeoPixels conditionally? For specific hardware revisions? Or turn on/off in settings?
There was a problem hiding this comment.
Yes, absolutely. Doing it for specific hardware revisions isn't necessary since it doesn't hurt to squirt NeoPixel colors into a disconnected GPIO pin. But allowing the user to disable the LEDs in software definitely makes sense. I'll make that easier to do.
| @@ -166,7 +245,12 @@ void initI2SRx() { | |||
|
|
|||
| // Initialize ADC | |||
| adc1_config_width(ADC_WIDTH_BIT_12); | |||
| adc1_config_channel_atten(ADC1_CHANNEL_6, ADC_ATTEN_DB_12); | |||
| if (hardware_version == HW_V2_0C) { | |||
There was a problem hiding this comment.
Maybe it is better approach to change it in settings?
There was a problem hiding this comment.
Why's that? This is a hardware dependent change, dependent on where the bias voltage has been set. v2.0c has a fixed bias at 0.56v, therefore requires a low output volume from the 818 so it doesn't clip into the negative, and therefore requires DB_0.
v2.0d uses your variable bias design and therefore should use the maximum signal to noise ratio design.
| result = dra->volume(8); | ||
| if (hardware_version == HW_V2_0C) { | ||
| // v2.0c has a lower input ADC range. | ||
| result = dra->volume(4); |
There was a problem hiding this comment.
Maybe it is better approach to change it in settings?
There was a problem hiding this comment.
Looks good to me with a few minor comments.
@VanceVagell, I believe features like VHF/UHF, NeoPixel support, volume, attenuation, etc., should be implemented in a consistent way. There are two possible approaches:
- The firmware automatically detects and sets them up. (and possible sends the state in GetVersion reply, if needed)
- The user can configure them through the Settings.
Feel free to merge or request further changes as you see fit.
Note: I personally prefer @SmittyHalibut’s approach of auto-detection, especially since we have plenty of ESP32 pins available. However, since you’ve started implementing the VHF/UHF switch in the settings, I wanted to raise this as a concern. ;) Also, I’d personally prefer to encode 'features' instead of board versions, but that’s just my take on it.
The only reason that VHF/UHF is implemented as a user option is because the SA818s don't report which band they are. There's no way to auto-detect this from the SA818 module. We could implement a way to specify at assemble time: jumper here for VHF, there for UHF. shrug That's not a horrible idea. |
|
Hi @SmittyHalibut, This is exactly what I mean! For example, we could short |
|
With the expansion headers, there aren't as many "unused" pins as it seems. We can definitely reclaim some of those from the headers if we want/need. But there isn't a glut of unused pins. That's why I originally put the first two version detect on the Sense pins; they were the only two that weren't already assigned. I think we'll run out of pins VERY quickly if we tried to make a pin for every feature. I believe we should keep with board versions. #myOpinion |
ESP32:
Added firmware support for:
PCB