search

esp-rs / esp-pacs

Peripheral Access Crates for Espressif SoCs and modules
Apache License 2.0
103 stars 29 forks source link

Fix `I2C/COMD/OPCODE` values #267

Closed playfulFence closed 1 month ago

playfulFence commented 1 month ago

Related to https://github.com/esp-rs/esp-hal/issues/1916

Data are taken from i2c_ll.h files for each chip in esp-idf (e.g. https://github.com/espressif/esp-idf/blob/master/components/hal/esp32c6/include/hal/i2c_ll.h#L45-L49)

burrbull commented 1 month ago

Slightly confusing that you write one enum and then replace it with another.

playfulFence commented 1 month ago

Honestly, I wasn't able to find information about how to edit some exact variant of enumeratedValues. If you could share it, I'd be grateful and will be happy to do it in a more elegant way.

But the reasoning behind the whole thing is simple: esp32 and esp32s2 have different OPCODE values than the rest of boards. Also in "original" version of SVD, so we need to patch all the chips anyway

burrbull commented 1 month ago

I try to say that it is better to remove adding OPCODE from i2c0.yaml to avoid replacing. esp32:

I2C0:
  COMD%s:
    OPCODE:
      Rstart: [0, RSTART opcode]
      Write: [1, WRITE opcode]
      Read: [2, READ opcode]
      Stop: [3, STOP opcode]
      End: [4, END opcode]
  _include:
    - ../../../common_patches/i2c0.yaml

esp32c6 and other:

I2C0:
  _include:
    - ../../../common_patches/i2c0.yaml
    - ../../../common_patches/i2c_opcode.yaml # contains default values
MabezDev commented 1 month ago

I try to say that it is better to remove adding OPCODE from i2c0.yaml to avoid replacing. esp32:

I2C0:
  COMD%s:
    OPCODE:
      Rstart: [0, RSTART opcode]
      Write: [1, WRITE opcode]
      Read: [2, READ opcode]
      Stop: [3, STOP opcode]
      End: [4, END opcode]
  _include:
    - ../../../common_patches/i2c0.yaml

esp32c6 and other:

I2C0:
  _include:
    - ../../../common_patches/i2c0.yaml
    - ../../../common_patches/i2c_opcode.yaml # contains default values

I think the current approach is fine, new chips will more than likely have the default and we won't need to apply the patch at all on them