Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Other): Fix Wrap_MXC_CAN_Init signature #1320

Merged

Conversation

trupples
Copy link
Contributor

@trupples trupples commented Jan 9, 2025

Description

Fixes a bug introduced by #1306, which added a Zephyr wrapper for MXC_CAN_Init, whose signature differs between the MAX32662 and MAX32690.

The previous implementation declared Wrap_MXC_CAN_Init as void and discarded the return value of the underlying MXC_CAN_Init calls. This is incompatible with the can_max32 driver which does error checking and expects an int return value.

While this is a "breaking" change, no code currently depends on this wrapper, because its signature is invalid and could not be properly used in the can_max32 driver.

How did this happen?

This is entirely my fault, as I was rushing to get some zephyr can_max32 changes in, on the last Friday before the winter holiday break. (I am now well aware of how stupid that is...)

While working on this wrapper, I used the following forks: trupples/msdk, trupples/hal_adi. I did catch this error and fixed it in hal_adi, but didn't push the same changes to msdk.

At that point, trupples/hal_adi had the correct implementation (int return), but trupples/msdk did not (void return).

I was able to build and test an application using the driver, because in zephyr I only referenced my hal_adi repo. In PR #1306 I claimed the wrapper is correct and tested, and the PR got accepted, but with the wrong code. The wrong code was then automatically copied to analogdevicesinc/hal_adi.

NB: should have listened to shouldideploy.today

Validation

A minimal example to test this on both MAX32662 and MAX32690 can be found at trupples/max32-can-validate. It unfortunately requires some fiddling around to get the WIP driver from innersource. Most importantly, the entire process is automated by validate.sh, which:

  1. Clones hal_adi (develop branch), msdk (the branch in this PR)
  2. Uses zephyr-hal.sh to properly copy files to hal_adi
  3. Prepares the west workspace
  4. Overwrites hal_adi with modified variant (this is the step I'm most doubtful of, because it intentionally bypasses west update to more easily apply the changes)
  5. Builds example for MAX32690 and MAX32662

Usage:

cd /path/to/west_workspace
git clone https://github.com/trupples/max32-can-validate/ validate
cd validate
./validate.sh

I have successfully run this, and I am quite confident that I am not wasting your time with a second bad PR, but I'd like a sanity check.

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • (Optional) Link any related GitHub issues using a keyword
  • (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

@github-actions github-actions bot added the Zephyr MSDK Zephyr related change. label Jan 9, 2025
@trupples trupples force-pushed the feat/zephyr-wrap-max32-can branch from 361c593 to 0749bf7 Compare January 9, 2025 17:09
@trupples trupples marked this pull request as ready for review January 9, 2025 17:16
Copy link
Contributor

@sihyung-maxim sihyung-maxim left a comment

Choose a reason for hiding this comment

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

No worries, thanks for catching this. Just an unfortunate consequence that we don't have workflows to check for everything.

Libraries/zephyr/MAX/Include/wrap_max32_can.h Outdated Show resolved Hide resolved
@trupples trupples force-pushed the feat/zephyr-wrap-max32-can branch from 0749bf7 to d97c713 Compare January 14, 2025 18:07
The previous implementation declared Wrap_MXC_CAN_Init as void
and discarded the return value of the underlying MXC_CAN_Init
calls. This is incompatible with the can_max32 driver which
does eror checking and expects an int return value.

While this is a "breaking" change, no code depends on this
wrapper, because its signature is invalid.

Signed-off-by: Ioan Dragomir <[email protected]>
@trupples trupples force-pushed the feat/zephyr-wrap-max32-can branch from d97c713 to 21347d3 Compare January 14, 2025 18:07
@sihyung-maxim
Copy link
Contributor

Interesting that the Build_Examples workflow is having trouble with your fork. I'm in the process updating this workflow at the moment, but merging this PR because the Zephyr files are not relevant with the Build_Examples workflow.

@sihyung-maxim sihyung-maxim merged commit 4f0d3d3 into analogdevicesinc:main Jan 17, 2025
7 of 8 checks passed
sihyung-maxim pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Zephyr MSDK Zephyr related change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants