Skip to content

Update libopeninv submodule to latest (jsphuebner 51a9e58)#239

Closed
wjcloudy wants to merge 1 commit into
damienmaguire:masterfrom
wjcloudy:update-libopeninv-latest
Closed

Update libopeninv submodule to latest (jsphuebner 51a9e58)#239
wjcloudy wants to merge 1 commit into
damienmaguire:masterfrom
wjcloudy:update-libopeninv-latest

Conversation

@wjcloudy

@wjcloudy wjcloudy commented Jun 20, 2026

Copy link
Copy Markdown

Summary

Bumps the libopeninv submodule from the currently-pinned 3a9c0594 to the current jsphuebner tip 51a9e58, and adapts the VCU. Hardware-validated (see Testing).

What the bump brings

libopeninv commits since 3a9c0594 include:

  • 100 kbps (Baud100) CAN bit timings
  • Indexed CAN-map transmit API + fix for the premature ~20-signal CAN-map limit
  • SDO refactor — user-space SDO objects dispatched via CanSdo::ProcessSDOSdoCommands

VCU changes in this PR

  1. CanSdo — drop the GetPendingUserspaceSdo() main-loop poll. The new ProcessSDO already routes user-space SDO objects (serial 0x5000 / command 0x5002) to SdoCommands from HandleRx, so plain CanSdo suffices.
    • The SDO command path (incl. parm_save) runs in CAN-RX interrupt context — that's libopeninv's default ProcessSDO behaviour, not VCU code. Confirmed working on hardware (param save persists across reboot).
  2. No Send() call-site changes — an earlier revision cast lengths to uint8_t to dodge an overload ambiguity; canhardware: remove ambiguous 2-arg Send overload (lets callers drop uint8_t casts) jsphuebner/libopeninv#56 (now in 51a9e58) removed the ambiguous overload, so the casts are gone and Send(id, u32buf, len) resolves cleanly.

clang-format

Brings i3LIM.cpp / leafbms.cpp / utils.cpp into line with the repo's pinned pre-commit hook (v20.1.8, --all-files), which flags pre-existing whitespace on every PR. Whitespace only, no functional change.

Testing

  • ✅ Builds clean for STM32F1 (make).
  • ✅ Bench-tested on a real VCU: Baud100 (100 kbps) CAN init works; Nissan Leaf inverter runs; SDO param save persists across reboot.
  • ⬜ Other peripherals (chargers, BMS, other inverters) not covered by this test.

@wjcloudy

Copy link
Copy Markdown
Author

Hardware update: flashed a build containing these changes onto a VCU — 100 kbps (Baud100) CAN init works, and a Nissan Leaf inverter runs correctly (exercises the cast Send() calls + CAN TX/RX on the new libopeninv, no regression). Still to validate before un-drafting: the web-UI SDO save / load / reset path under the new CAN-RX-ISR dispatch. Other peripherals (chargers / BMS / other inverters) not covered by this test.

@wjcloudy

Copy link
Copy Markdown
Author

Further hardware validation: SDO param save now confirmed — changed a parameter, saved over SDO, rebooted, and the value stuck. So the CAN-RX-ISR ProcessUserSpaceSdo path (including the parm_save() flash write) works in practice. Remaining before this leaves draft is just broader peripheral coverage (chargers / BMS / other inverters).

@wjcloudy
wjcloudy force-pushed the update-libopeninv-latest branch 2 times, most recently from b2db312 to 07b6139 Compare June 20, 2026 19:52
@jsphuebner

jsphuebner commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

The ISR context for ProcessSDO wasn't fully intended, just a logical consequence. It should be fine for short-running handlers. Anything long-running should be deferred to main loop.
Having to explicitely cast to uint8_t for Send() is a bit unfortunate, API change requests welcome

@wjcloudy

wjcloudy commented Jun 20, 2026

Copy link
Copy Markdown
Author

Thanks for the feedback — addressed both:

ISR context / long-running handlers: good point, and looking closer our VcuCanSdo override was actually redundant. The base CanSdo::ProcessSDO already routes user-space objects (serial 0x5000 / command 0x5002) to SdoCommands from HandleRx, so the flash save runs in the ISR via libopeninv's own default path — not our code. I've dropped the override; the PR now just removes the old GetPendingUserspaceSdo poll and uses plain CanSdo. So if the long-running SDO commands (save/load) should be deferred to the main loop, that looks like a libopeninv-level decision in ProcessSDO. Happy to add a deferring CanSdo subclass on the VCU side instead if you'd prefer that.

Send() cast: @jsphuebner took you up on the API-change offer — opened jsphuebner/libopeninv#56 (draft). Dropping the unused bool forceExt from the 2-arg Send convenience removes the ambiguity, so Send(id, u32buf, len) resolves cleanly with no cast (verified against this branch). If that lands, I'll bump the submodule here and revert all the (uint8_t) casts.

@wjcloudy
wjcloudy force-pushed the update-libopeninv-latest branch from c6b1621 to 1288efe Compare June 21, 2026 09:22
@wjcloudy wjcloudy changed the title Update libopeninv submodule to latest (jsphuebner 6b78f96) Update libopeninv submodule to latest (jsphuebner 51a9e58) Jun 21, 2026
@wjcloudy

wjcloudy commented Jun 21, 2026

Copy link
Copy Markdown
Author

Updated: jsphuebner/libopeninv#56 is merged, so I've moved the pin forward to 51a9e58 and dropped all 76 (uint8_t) castsSend(id, u32buf, len) now resolves cleanly. The PR is down to the submodule bump, the CanSdo simplification, and a little pre-existing-whitespace clang-format. Builds clean without casts. Thanks @jsphuebner for the speedy merge!

@jsphuebner

Copy link
Copy Markdown
Collaborator

You can also drop SdoCommands::SetCanMap(cm); as that is done within CanSdo

Bump the libopeninv submodule to current jsphuebner tip (51a9e58 — adds
Baud100 / 33.3k, the SDO refactor, and the Send-overload fix) and adapt
the VCU:

- CanSdo: drop the GetPendingUserspaceSdo() main-loop poll. The new
  libopeninv ProcessSDO routes user-space SDO objects (serial 0x5000 /
  command 0x5002) to SdoCommands from HandleRx, so plain CanSdo suffices.

No Send() call-site changes needed: libopeninv#56 (in 51a9e58) removed
the ambiguous 2-arg Send overload, so Send(id, u32buf, len) resolves
cleanly without casts.

clang-format note: brings i3LIM.cpp / leafbms.cpp / utils.cpp into line
with the repo's pinned pre-commit hook (v20.1.8, --all-files), which
flags pre-existing whitespace on every PR. Whitespace only, no
functional change.

Builds clean for the STM32F1 target.
@wjcloudy
wjcloudy force-pushed the update-libopeninv-latest branch from 1288efe to 73db875 Compare June 21, 2026 10:04
@wjcloudy

Copy link
Copy Markdown
Author

Good catch — done (also dropped the now-orphaned #include "sdocommands.h" since that was its only use). Pushed.

@jsphuebner jsphuebner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks legit

@Tom-evnut

Copy link
Copy Markdown
Collaborator

Sorry any pushes should go to vehicle testing as per https://github.com/damienmaguire/Stm32-vcu/blob/master/CONTRIBUTING.md

@wjcloudy

Copy link
Copy Markdown
Author

Re-targeted to Vehicle_Testing per CONTRIBUTING.md — superseded by #241 (same change, based on Vehicle_Testing).

@wjcloudy wjcloudy closed this Jun 21, 2026
@wjcloudy
wjcloudy deleted the update-libopeninv-latest branch June 21, 2026 17:55
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.

3 participants