Skip to content

Conversation

@solomondg
Copy link
Contributor

Changes:

  • Add ODriveESP32TWAI.hpp interface
  • Add IS_ESP32_TWAI option to SineWaveCAN example
  • Add esp32 PlatformIO target

This PR supersedes #11 .

Not tested yet - will desk test ASAP.

Changes:
- Add ODriveESP32TWAI.hpp interface that calls twai_transmit/twai_receive directly
- Add IS_ESP32_TWAI option to SineWaveCAN example
- Add esp32 PlatformIO target
@CLAassistant
Copy link

CLAassistant commented Jan 6, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@samuelsadok samuelsadok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending desk test.

twai_message_t tx_msg = {};
tx_msg.identifier = id;
tx_msg.data_length_code = length;
tx_msg.extd = (id > 0x7FF) ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing drivers use id & 0x80000000 as the extended-id flag, so we should do the same here.

@solomondg
Copy link
Contributor Author

@samuelsadok Desk tested OK w/ ESP32-WROOM-32 devkit + SN65HVD230 breakout. Hit your comments. Should be good to merge if all looks good to you. Only possible change is moving the TWAI setup code (namely the large case/switch statement) to ODriveESP32TWAI.hpp to reduce visual noise in SineWaveCAN.ino

Copy link
Member

@samuelsadok samuelsadok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok looks good to me.

The setupCan() is a bit of visual noise, that is true, but that is also true for the larger section that does the hardware setup. The thing is that it's quite board dependent, so it can't be straight-up in a library header (you could even have multiple CAN interfaces, so you can't just make simple GPIO/baudrate defines before the library include).

What you could do, is just move the generic portion (the switch statement) over to a helper function in the header. Up to you if you want to do that in this PR.

Long term maybe we'll start putting each of these sections in its separate board header that users can copy and adapt for their board.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants