Fix save config (#27)

* Fix save config

* Fix save config when no audio is playing

* Fix build issue with the github workflow compiler

firmware/code/configuration_manager.c

@@ -54,7 +54,7 @@ static const default_configuration default_config = {
         .filter = { FILTER_CONFIGURATION, sizeof(default_config.filters) },
         .f1  = { PEAKING,    {0},    38.5, -21.0,  1.4  },
         .f2  = { PEAKING,    {0},    60,    -6.7,  0.5  },
-        .f3  = { LOWSHELF,   {0},    105,    5.5,  0.71 },
+        .f3  = { LOWSHELF,   {0},    105,    2.0,  0.71 },
         .f4  = { PEAKING,    {0},    280,   -3.5,  1.1  },
         .f5  = { PEAKING,    {0},    350,   -1.6,  6.0  },
         .f6  = { PEAKING,    {0},    425,    7.8,  1.3  },
@@ -66,9 +66,15 @@ static const default_configuration default_config = {
         .f12 = { PEAKING,    {0},   3430,  -12.2,  2.0  },
         .f13 = { PEAKING,    {0},   4800,    4.0,  2.0  },
         .f14 = { PEAKING,    {0},   6200,  -15.0,  3.0  },
-        .f15 = { HIGHSHELF,  {0},  12000,   -6.0,  0.71 }
+        .f15 = { HIGHSHELF,  {0},  12000,   -3.0,  0.71 }
     },
-    .preprocessing = { .header = { PREPROCESSING_CONFIGURATION, sizeof(default_config.preprocessing) }, -0.08f, true, {0} }
+    .preprocessing = { 
+        .header = { PREPROCESSING_CONFIGURATION, sizeof(default_config.preprocessing) }, 
+        -0.376265f,      // pre-EQ gain of -4.1dB
+        0.4125f,       // post-EQ gain, set to ~3dB (1.4x, less the 1 that is added when config is applied)
+        true, 
+        {0} 
+    }
 };
 
 // Grab the last 4k page of flash for our configuration strutures.
@@ -90,6 +96,13 @@ static bool reload_config = false;
 static uint16_t write_offset = 0;
 static uint16_t read_offset = 0;
 
+typedef enum {
+    NormalOperation,
+    SaveRequested,
+    Saving
+} State;
+static State saveState = NormalOperation;
+
 bool validate_filter_configuration(filter_configuration_tlv *filters)
 {
     if (filters->header.type != FILTER_CONFIGURATION) {
@@ -316,6 +329,7 @@ bool apply_configuration(tlv_header *config) {
             case PREPROCESSING_CONFIGURATION: {
                 preprocessing_configuration_tlv* preprocessing_config = (preprocessing_configuration_tlv*) tlv;
                 preprocessing.preamp = fix3_28_from_flt(1.0f + preprocessing_config->preamp);
+                preprocessing.postEQGain = fix3_28_from_flt(1.0f + preprocessing_config->postEQGain);
                 preprocessing.reverse_stereo = preprocessing_config->reverse_stereo;
                 break;
             }
@@ -350,32 +364,49 @@ void load_config() {
 }
 
 #ifndef TEST_TARGET
-bool __no_inline_not_in_flash_func(save_configuration)() {
+bool __no_inline_not_in_flash_func(save_config)() {
     const uint8_t active_configuration = inactive_working_configuration ? 0 : 1;
     tlv_header* config = (tlv_header*) working_configuration[active_configuration];
 
-    if (validate_configuration(config)) {
-        power_down_dac();
+    switch (saveState) {
+        case SaveRequested:
+            if (validate_configuration(config)) {      
+                /* Turn the DAC off so we don't make a huge noise when disrupting
+                real time audio operation. */
+                power_down_dac();
 
-        const size_t config_length = config->length - ((size_t)config->value - (size_t)config);
-        // Write data to flash
-        uint8_t flash_buffer[CFG_BUFFER_SIZE];
-        flash_header_tlv* flash_header = (flash_header_tlv*) flash_buffer;
-        flash_header->header.type = FLASH_HEADER;
-        flash_header->header.length = sizeof(flash_header_tlv) + config_length;
-        flash_header->magic = FLASH_MAGIC;
-        flash_header->version = CONFIG_VERSION;
-        memcpy((void*)(flash_header->tlvs), config->value, config_length);
+                const size_t config_length = config->length - ((size_t)config->value - (size_t)config);
+                // Write data to flash
+                uint8_t flash_buffer[CFG_BUFFER_SIZE];
+                flash_header_tlv* flash_header = (flash_header_tlv*) flash_buffer;
+                flash_header->header.type = FLASH_HEADER;
+                flash_header->header.length = sizeof(flash_header_tlv) + config_length;
+                flash_header->magic = FLASH_MAGIC;
+                flash_header->version = CONFIG_VERSION;
+                memcpy((void*)(flash_header->tlvs), config->value, config_length);
 
-        uint32_t ints = save_and_disable_interrupts();
-        flash_range_erase(USER_CONFIGURATION_OFFSET, FLASH_SECTOR_SIZE);
-        flash_range_program(USER_CONFIGURATION_OFFSET, flash_buffer, CFG_BUFFER_SIZE);
-        restore_interrupts(ints);
+                uint32_t ints = save_and_disable_interrupts();
+                flash_range_erase(USER_CONFIGURATION_OFFSET, FLASH_SECTOR_SIZE);
+                flash_range_program(USER_CONFIGURATION_OFFSET, flash_buffer, CFG_BUFFER_SIZE);
+                restore_interrupts(ints);
+                saveState = Saving;
 
-        power_up_dac();
-
-        return true;
+                // Return true, so the caller skips processing audio
+                return true;
+            }
+            // Validation failed, give up.
+            saveState = NormalOperation;
+            break;
+        case Saving:
+            /* Turn the DAC off so we don't make a huge noise when disrupting
+            real time audio operation. */
+            power_up_dac();
+            saveState = NormalOperation;
+            return false;
+        default:
+            break;
     }
+
     return false;
 }
 
@@ -401,7 +432,14 @@ bool process_cmd(tlv_header* cmd) {
             }
             break;
         case SAVE_CONFIGURATION: {
-            if (cmd->length == 4 && save_configuration()) {
+            if (cmd->length == 4) {
+                saveState = SaveRequested;
+                if (audio_state.interface == 0) {
+                    // The OS will configure the alternate "zero" interface when the device is not in use
+                    // in this sate we can write to flash now. Otherwise, defer the save until we get the next
+                    // usb packet.
+                    save_config();
+                }
                 result->type = OK;
                 result->length = 4;
                 return true;

firmware/code/configuration_manager.h

@@ -52,6 +52,7 @@ void config_in_packet(struct usb_endpoint *ep);
 void config_out_packet(struct usb_endpoint *ep);
 void configuration_ep_on_cancel(struct usb_endpoint *ep);
 extern void load_config();
+extern bool save_config();
 extern void apply_config_changes();
 
 #endif // CONFIGURATION_MANAGER_H

firmware/code/configuration_types.h

@@ -17,8 +17,8 @@
 #include <stdint.h>
 
 #define FLASH_MAGIC 0x2E8AFEDD
-#define CONFIG_VERSION 3
-#define MINIMUM_CONFIG_VERSION 3
+#define CONFIG_VERSION 4
+#define MINIMUM_CONFIG_VERSION 4
 
 enum structure_types {
     // Commands/Responses, these are container TLVs. The Value will be a set of TLV structures.
@@ -98,9 +98,13 @@ typedef struct __attribute__((__packed__)) _flash_header_tlv {
     const uint8_t tlvs[0];
 } flash_header_tlv;
 
+/// @brief Holds values relating to processing surrounding the EQ calculation.
 typedef struct __attribute__((__packed__)) _preprocessing_configuration_tlv {
     tlv_header header;
+    /// @brief Gain applied to input signal before EQ chain. Use to avoid clipping due to overflow in the biquad filters of the EQ.
     float preamp;
+    /// @brief Gain applied to the output of the EQ chain. Used to set output volume.
+    float postEQGain;
     uint8_t reverse_stereo;
     uint8_t reserved[3];
 } preprocessing_configuration_tlv;

firmware/code/fix16.h

@@ -41,7 +41,7 @@ static const fix3_28_t fix16_zero = 0x00000000;
 
 static inline fix3_28_t norm_fix3_28_from_s16sample(int16_t);
 
-static inline int16_t norm_fix3_28_to_s16sample(fix3_28_t);
+static inline int32_t norm_fix3_28_to_s16sample(fix3_28_t);
 
 static inline fix3_28_t fix3_28_from_flt(float);
 

firmware/code/fix16.inl

@@ -32,18 +32,18 @@ static inline fix3_28_t norm_fix3_28_from_s16sample(int16_t a) {
     /* So, we're using a Q3.28 fixed point system here, and we want the incoming
        audio signal to be represented as a number between -1 and 1. To do this,
        we need the 16-bit value to map to the 28-bit right-of-decimal field in
-       our fixed point number. 28-16 = 12, so we shift the incoming value by
-       that much to covert it to the desired Q3.28 format and do the normalization
-       all in one go.
+       our fixed point number. 28-16 = 12 + the sign bit = 13, so we shift the
+       incoming value by that much to covert it to the desired Q3.28 format and
+       do the normalization all in one go.
     */
-    return (fix3_28_t)a << 12;
+    return (fix3_28_t)a << 13;
 }
 
 /// @brief Convert fixed point samples into signed integer. Used to convert
 ///        calculated sample to one that the DAC can understand.
 /// @param a
 /// @return Signed 16-bit integer.
-static inline int16_t norm_fix3_28_to_s16sample(fix3_28_t a) {
+static inline int32_t norm_fix3_28_to_s16sample(fix3_28_t a) {
     // Handle rounding up front, adding one can cause an overflow/underflow
 
     // It's not clear exactly how this works, so we'll disable it for now.
@@ -56,22 +56,20 @@ static inline int16_t norm_fix3_28_to_s16sample(fix3_28_t a) {
     */
 
     // Saturate the value if an overflow has occurred
-    uint32_t upper = (a >> 30);
+    uint32_t upper = (a >> 29);
     if (a < 0) {
-        if (~upper)
-        {
-            return SHRT_MIN;
+        if (~upper) {
+            return 0xff800000;
         }
     } else {
-        if (upper)
-        {
-            return SHRT_MAX;
+        if (upper) {
+            return 0x00efffff;
         }
     }
     /* When we converted the USB audio sample to a fixed point number, we applied
        a normalization, or a gain of 1/65536. To convert it back, we can undo that
-       by shifting it back by the same amount we shifted it in the first place. */
-    return (a >> 12);
+       by shifting it but we output 24bts, so the shift is reduced. */
+    return (a >> 6);
 }
 
 static inline fix3_28_t fix3_28_from_flt(float a) {

firmware/code/run.c

@@ -52,10 +52,12 @@ static uint8_t *userbuf;
 audio_state_config audio_state = {
     .freq = 48000,
     .de_emphasis_frequency = 0x1, // 48khz
+    .interface = 0
 };
 
 preprocessing_config preprocessing = {
     .preamp = fix16_one,
+    .postEQGain = fix16_one,
     .reverse_stereo = false
 };
 
@@ -124,6 +126,18 @@ static void __no_inline_not_in_flash_func(_as_audio_packet)(struct usb_endpoint
     int32_t *out = (int32_t *) userbuf;
     int samples = usb_buffer->data_len / 2;
 
+    // Make sure core 1 is ready for us.
+    multicore_fifo_pop_blocking();
+
+    if (save_config()) {
+        // Skip processing while we are writing to flash
+        multicore_fifo_push_blocking(CORE0_ABORTED);
+        // keep on truckin'
+        usb_grow_transfer(ep->current_transfer, 1);
+        usb_packet_done(ep);
+        return;
+    }
+
     if (preprocessing.reverse_stereo) {
         for (int i = 0; i < samples; i+=2) {
             out[i] = in[i+1];
@@ -135,21 +149,22 @@ static void __no_inline_not_in_flash_func(_as_audio_packet)(struct usb_endpoint
             out[i] = in[i];
     }
  
-    // Make sure core 1 is ready for us.
-    multicore_fifo_pop_blocking();
     multicore_fifo_push_blocking(CORE0_READY);
     multicore_fifo_push_blocking(samples);
 
-    for (int j = 0; j < filter_stages; j++) {
-        // Left channel filter
-        for (int i = 0; i < samples; i += 2) {
-            fix3_28_t x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preprocessing.preamp);
 
+    // Left channel filter
+    for (int i = 0; i < samples; i += 2) {
+        fix3_28_t x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preprocessing.preamp);
+        for (int j = 0; j < filter_stages; j++) {
             x_f16 = bqf_transform(x_f16, &bqf_filters_left[j],
                 &bqf_filters_mem_left[j]);
-
-            out[i] = (int32_t) norm_fix3_28_to_s16sample(x_f16);
         }
+
+        /* Apply post-EQ gain. */
+        x_f16 = fix16_mul( x_f16, preprocessing.postEQGain);
+
+        out[i] = (int32_t) norm_fix3_28_to_s16sample(x_f16);
     }
 
     // Block until core 1 has finished transforming the data
@@ -182,20 +197,23 @@ void __no_inline_not_in_flash_func(core1_entry)() {
 
         // Block until the userbuf is filled with data
         uint32_t ready = multicore_fifo_pop_blocking();
-        while (ready != CORE0_READY)
-            ready = multicore_fifo_pop_blocking();
+        if (ready == CORE0_ABORTED) continue;
         
         const uint32_t samples = multicore_fifo_pop_blocking();
 
-        for (int j = 0; j < filter_stages; j++) {
-            for (int i = 1; i < samples; i += 2) {
-                fix3_28_t x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preprocessing.preamp);
-
+        /* Right channel EQ. */
+        for (int i = 1; i < samples; i += 2) {
+            /* Apply EQ pre-filter gain to avoid clipping. */
+            fix3_28_t x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preprocessing.preamp);
+            /* Apply the biquad filters one by one. */
+            for (int j = 0; j < filter_stages; j++) {
                 x_f16 = bqf_transform(x_f16, &bqf_filters_right[j],
                     &bqf_filters_mem_right[j]);
-
-                out[i] = (int16_t) norm_fix3_28_to_s16sample(x_f16);
             }
+            /* Apply post-EQ gain. */
+            x_f16 = fix16_mul( x_f16, preprocessing.postEQGain);
+
+            out[i] = (int32_t) norm_fix3_28_to_s16sample(x_f16);
         }
 
         // Signal to core 0 that the data has all been transformed
@@ -273,9 +291,9 @@ void setup() {
     // Same here, pal. Hands off.
     sleep_ms(100);
 
-    // Set data format to 16 bit right justified, MSB first
+    // Set data format to 24 bit right justified, MSB first
     buf[0] = 67;   // register addr
-    buf[1] = 0x03; // data
+    buf[1] = 0x02; // data
     i2c_write_blocking(i2c0, PCM_I2C_ADDR, buf, 2, false);
 
     i2s_write_obj.sck_pin = PCM3060_DAC_SCK_PIN;
@@ -752,6 +770,7 @@ static const struct usb_transfer_type _audio_cmd_transfer_type = {
 
 static bool as_set_alternate(struct usb_interface *interface, uint alt) {
     assert(interface == &as_op_interface);
+    audio_state.interface = alt;
     switch (alt) {
         case 0: power_down_dac(); return true;
         case 1: power_up_dac(); return true;
@@ -960,7 +979,7 @@ void power_down_dac() {
     i2c_write_blocking(i2c0, PCM_I2C_ADDR, buf, 2, false);
 }
 
-void power_up_dac() {
+void power_up_dac() {    
     uint8_t buf[2];
     buf[0] = 64; // register addr
     buf[1] = 0xE0; // DAC normal mode

firmware/code/run.h

@@ -76,6 +76,7 @@ typedef struct _audio_state_config {
         int16_t _target_pcm3060_registers;
     };
     int16_t pcm3060_registers;
+    int8_t interface;
 } audio_state_config;
 extern audio_state_config audio_state;
 
@@ -110,6 +111,8 @@ typedef struct _audio_device_config {
 
 typedef struct _preprocessing_config {
     fix3_28_t preamp;
+    /// @brief Apply this gain after applying EQ, to set output volume without causing overflow in the EQ calculations.
+    fix3_28_t postEQGain;
     int reverse_stereo;
 } preprocessing_config;
 
@@ -147,6 +150,7 @@ static char *descriptor_strings[] = {
 #define SAMPLING_FREQ (CODEC_FREQ / 192)
 
 #define CORE0_READY 19813219
+#define CORE0_ABORTED 91231891
 #define CORE1_READY 72965426
 
 /*****************************************************************************

firmware/tools/CMakeLists.txt

@@ -6,7 +6,6 @@ set(CMAKE_CXX_STANDARD 17)
 
 add_executable(filter_test
     filter_test.c
-    ../code/fix16.c
     ../code/bqf.c
     ../code/configuration_manager.c
 )

firmware/tools/README.md

@@ -17,7 +17,7 @@ Run `filter_test` to process the PCM samples. The `filter_test` program takes tw
 You can listen to the PCM files using ffplay (which is usually included with ffmpeg):
 
 ```
-ffplay -f s16le -ar 48000 -ac 2 output.pcm
+ffplay -f s24le -ar 48000 -ac 2 output.pcm
 ```
 
 If there are no obvious problems, go ahead and flash your firmware.

firmware/tools/filter_test.c

@@ -32,7 +32,7 @@ int main(int argc, char* argv[])
     // we dont need to store the whole input and output files in memory.
     int samples = input_size / 2;
     int16_t *in = (int16_t *) calloc(samples, sizeof(int16_t));
-    int16_t *out = (int16_t *) calloc(samples, sizeof(int16_t));
+    int32_t *out = (int32_t *) calloc(samples, sizeof(int32_t));
 
     fread(in, samples, sizeof(int16_t), input);
     fclose(input);
@@ -54,31 +54,39 @@ int main(int argc, char* argv[])
         out[i] = in[i];
     }
 
-    for (int j = 0; j < filter_stages; j++)
-    {
-        for (int i = 0; i < samples; i ++)
-        {
-            // Left channel filter
-            fix16_t x_f16 = fix16_from_s16sample((int16_t) out[i]);
+    const fix3_28_t preamp = fix3_28_from_flt(0.92f);
 
+    for (int i = 0; i < samples; i ++)
+    {
+        // Left channel filter
+        fix3_28_t x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preamp);
+
+        for (int j = 0; j < filter_stages; j++)
+        {
             x_f16 = bqf_transform(x_f16, &bqf_filters_left[j],
                 &bqf_filters_mem_left[j]);
+        }
 
-            out[i] = (int32_t) fix16_to_s16sample(x_f16);
+        out[i] = (int32_t) norm_fix3_28_to_s16sample(x_f16);
 
-            // Right channel filter
-            i++;
-            x_f16 = fix16_from_s16sample((int16_t) out[i]);
+        // Right channel filter
+        i++;
+        x_f16 = fix16_mul(norm_fix3_28_from_s16sample((int16_t) out[i]), preamp);
 
+        for (int j = 0; j < filter_stages; j++)
+        {
             x_f16 = bqf_transform(x_f16, &bqf_filters_right[j],
                 &bqf_filters_mem_right[j]);
-
-            out[i] = (int16_t) fix16_to_s16sample(x_f16);
         }
+
+        out[i] = (int32_t) norm_fix3_28_to_s16sample(x_f16);
+        //printf("%08x\n", out[i]);
     }
 
     // Write out the processed audio.
-    fwrite(out, samples, sizeof(int16_t), output);
+    for (int i=0; i<samples; i++) {
+        fwrite(&out[i], 3, sizeof(int8_t), output);
+    }
     fclose(output);
 
     free(in);