LEDC.low_speed_mode error 258

[Rashidwi]

I've been getting:

E (757) ledc: ledc_timer_config(785): speed_mode argument is invalid Unable to set ledc timer config. err=258

When setting low_speed_mode on ESP32S2.

Further investigations suggests that the values are reversed for high and low speed in LEDC.ex:

  @doc """
  Convenience function for setting ledc speed mode
  """
  @spec high_speed_mode() :: 0
  def high_speed_mode(),
    do: 0

  @doc """
  Convenience function for setting ledc speed mode
  """
  @spec low_speed_mode() :: 1
  def low_speed_mode(),
    do: 1

As the ESP32S2 doesn't support high_speed, I changed the value to 0 and the problem is solved.

[petermm]

nice find!

Seems like it's an enumerator so 0 is high speed_mode on boards with both modes, and 0 is low speed_mode on boards with only that mode.

eg esp32-s2: https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/api-reference/peripherals/ledc.html#id3

esp32: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/ledc.html#id3

there is LEDC_SPEED_MODE_MAX for us to create some kind of logic to sort out the mess. we probably need some helpers in the NIF https://github.com/atomvm/AtomVM/blob/main/src/platforms/esp32/components/avm_builtins/ledc_nif.c

We probably want to return 0 for both modes on esp32-s2 and 0/1 on esp32.

[UncleGrumpy]

Thanks for looking into that @petermm! I suspected this was the situation.

I think we need to use a speed_mode table with interop_atom_term_select_int and return the esp-idf macros for the value. This way we can ensure the correct mode, and use a consistent input from the Erlang module. Then low will always select low speed - regardless of the chip, and high can always represent high speed, regardless of the actual enum values.

[UncleGrumpy]

I looked into this driver a bit, it desperately needs some work. There are unsafe atom creations throughout (see issue #1442), I would try to fix this, but am waiting for a review of #1457 to be sure that I am meeting all of the criteria for issue #1442.