Moddable-OpenSource / moddable

Tools for developers to create truly open IoT products using standard JavaScript on low cost microcontrollers.
http://www.moddable.com
1.34k stars 235 forks source link

Instantiation Serial with `onReadable/onWritable` callback fails quietly #931

Closed meganetaaan closed 2 years ago

meganetaaan commented 2 years ago

Description Program terminates immediately upon instantiation of Serial (from embedded:io/serial).

Steps to Reproduce

  1. Instantiate Serial that has onReadable or onWritable callback
import Serial from 'embedded:io/serial'

new Serial({
  // For M5Stack Core2 Grove PortC
  receive: 13,
  transmit: 14,
  baud: 1_000_000,
  port: 2,
  format: 'number',
  onReadable: function(bytes) {
    while (bytes--) {
      this.read()
    }
  },
})
{
  "include": [
    "$(MODDABLE)/modules/io/manifest.json"
  ],
  "modules": {
    "*": [
      "./main"
    ]
  }
}

Other information

After some print debug I found uart_enable_rx_intr crashing quietly.

phoddie commented 2 years ago

Curious. ECMA-419 Serial has been working very reliably. Perhaps something changed in the ESP-IDF that led to this. I'll try to reproduce it. Can you confirm that you are running ESP-IDF v4.4.2?

meganetaaan commented 2 years ago

Thank you. Yes I'm using

phoddie commented 2 years ago

Yes, it is an ESP-IDF change. :(

In components/driver/uart.c you can change two functions (my changes are marked with @@):

esp_err_t uart_disable_intr_mask(uart_port_t uart_num, uint32_t disable_mask)
{
    ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
    UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
    if (p_uart_obj[uart_num]) //@@
        p_uart_obj[uart_num]->rx_int_usr_mask &= ~disable_mask;
    uart_hal_disable_intr_mask(&(uart_context[uart_num].hal), disable_mask);
    UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
    return ESP_OK;
}

esp_err_t uart_enable_intr_mask(uart_port_t uart_num, uint32_t enable_mask)
{
    ESP_RETURN_ON_FALSE((uart_num < UART_NUM_MAX), ESP_FAIL, UART_TAG, "uart_num error");
    UART_ENTER_CRITICAL(&(uart_context[uart_num].spinlock));
    /* Keep track of the interrupt toggling. In fact, without such variable,
     * once the RX buffer is full and the RX interrupts disabled, it is
     * impossible what was the previous state (enabled/disabled) of these
     * interrupt masks. Thus, this will be very particularly handy when
     * emptying a filled RX buffer. */
    if (p_uart_obj[uart_num]) //@@
        p_uart_obj[uart_num]->rx_int_usr_mask |= enable_mask;
    uart_hal_clr_intsts_mask(&(uart_context[uart_num].hal), enable_mask);
    uart_hal_ena_intr_mask(&(uart_context[uart_num].hal), enable_mask);
    UART_EXIT_CRITICAL(&(uart_context[uart_num].spinlock));
    return ESP_OK;
}

The very long comment above hints at what is going on. It looks like a shadow copy of the UART settings is now being maintained in RAM for some reason. This copy is in p_uart_obj. But, that is only initialized when calling uart_driver_install. (The uart.c code checks for this in some places, such as uart_set_tx_empty_threshold, but not all).

The solution may be to simply inline versions of the UART interrupt enable/disable functions to avoid the increasingly complex uart.c. I'll have to think about that.

If you could try the proposed changes to see if they work for you, that would be very helpful. Thank you.

meganetaaan commented 2 years ago

@phoddie Thank you for your investigation. Your fix above worked!

The solution may be to simply inline versions of the UART interrupt enable/disable functions to avoid the increasingly complex uart.c. I'll have to think about that.

Yes, I think it is better to have simple and enough code to meet the needs since the development of the ESP-IDF is so fast that often destructive changes are made even in relatively lower-level APIs.

phoddie commented 2 years ago

Thanks for confirming that it works for you too. I'm not sure I understand the API layers in the ESP-IDF Serial. Maybe it is changes over time. I've reworked the ECMA-419 serial implementation for ESP32 to bypass these troublesome functions. That should be available tomorrow.

phoddie commented 2 years ago

A fix is now available that does not required changes to ESP-IDF.