Changes required in STM CAN read API #14677

MubeenHCLite · 2021-05-19T06:21:53Z

Summary of changes

Added working functionality for both bxCAN and FDCAN where the CAN can accept filter IDs and work as desired.
FIFO selection in case of can_read is not supported as the can_filter sets FIFO0 as default FIFO, and the same is updated.

Impact of changes

Migration actions required

Documentation

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

Tests were carried out on both FDCAN and bxCAN, where multiple IDs were updated to be filtered on multiple CAN instances.
Results were verified that the filtering is as required.

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2021-05-19T06:30:15Z

@MubeenHCLite, thank you for your changes.
@ARMmbed/team-st-mcd @ARMmbed/mbed-os-maintainers please review.

0xc0170

otherwise LGTM

0xc0170 · 2021-05-21T12:50:09Z

targets/TARGET_STM/can_api.c

@@ -956,8 +958,10 @@ int can_write(can_t *obj, CAN_Message msg, int cc)

 int can_read(can_t *obj, CAN_Message *msg, int handle)
 {
+


this empty space is not needed?

@MubeenHCLite ping :-)

0xc0170 · 2021-05-21T12:51:48Z

@MubeenHCLite Please fill in the sections in the pull request template (PR type, how was it tested)

adbridge · 2021-05-24T13:48:25Z

@MubeenHCLite could you please fill in the PR template header correctly as per the guidelines provided.

targets/TARGET_STM/can_api.c

ciarmcom · 2021-06-01T03:50:26Z

This pull request has automatically been marked as stale because it has had no recent activity. , please complete review of the changes to move the PR forward. Thank you for your contributions.

mergify · 2021-06-01T08:03:56Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

0xc0170 · 2021-06-11T12:40:10Z

Please rebase instead of merging (clean linear history). Commit b6cab7 should not be in the pull request

0xc0170 · 2021-06-14T09:26:20Z

Can this be rebased and we can proceed?

MubeenHCLite · 2021-06-14T13:06:20Z

I have rebased the branch to current master head

jeromecoutant

Comment from @cindyli-13 not fully addressed.
Seems that multiple filter configuration can be supported

MubeenHCLite · 2021-06-14T13:16:09Z

Comment from @cindyli-13 not fully addressed.
Seems that multiple filter configuration can be supported

@cindyli-13 has already confirmed it is working fine

jeromecoutant · 2021-06-14T13:21:15Z

@cindyli-13 has already confirmed it is working fine

So maybe comments like
"message filter handle is not supported for STM controllers"
Have to be reconsidered ?

mergify · 2021-06-14T14:19:54Z

This PR cannot be merged due to conflicts. Please rebase to resolve them.

cindyli-13 · 2021-06-14T14:25:51Z

Comment from @cindyli-13 not fully addressed.
Seems that multiple filter configuration can be supported

@cindyli-13 has already confirmed it is working fine

Sorry if I wasn't clear earlier. I tested using a different Mbed patch and can confirm that I am able to configure multiple CAN filters. However I think that this PR will prevent the ability to configure multiple CAN filters because the can_filter() function will exit without configuring a filter if the filter handle is not 0.

MubeenHCLite · 2021-06-15T06:54:56Z

I will complete the implementation shortly and submit the PR

MubeenHCLite · 2021-06-21T03:31:40Z

Added working code to accept filter IDs for both bxCAN and FDCAN. Tested the patch on multiple can instances and results are as desired.

Note : Currently there is no way we can configure the number of standard/extended IDs to be filtered in FDCAN (List size Standard, list Size extended, refer RM), hence they are by default set to maximum possible values.

pilotak · 2021-06-23T08:24:37Z

I can confirm that it works and fixes the handle addressing

Here is a test code, only 0xFF0080 and 0xFF0580 is received

#include <mbed.h>
CAN can(PB_8, PB_9);
DigitalOut can_sleep(PB_7, 0);

#define ID   0xFF0080
#define ID2  0xFF0580
#define MASK 0xFFFFFF

int main() {
    can.frequency(250000);
    can.mode(CAN::Normal);
    can.filter(ID, MASK, CANExtended, 0);
    can.filter(ID2, MASK, CANExtended, 1);

    CANMessage msg;
    printf("start\n");

    while (1) {
        if (can.read(msg)) {
            printf("Message received ID: %X\n", msg.id);
        }
    }
}

It would be nice if following warnings could be fixed as well

[Warning] stm32g4xx_hal_fdcan.h@1327,49: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1327,68: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1325,61: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1325,84: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1331,46: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1331,65: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1325,61: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]
[Warning] stm32g4xx_hal_fdcan.h@1325,84: comparison of integer expressions of different signedness: 'int' and 'unsigned int' [-Wsign-compare]

MubeenHCLite · 2021-06-23T14:47:38Z

@pilotak
Thanks for the confirmation on working status.

We will take up resolving all the warnings in a separate issue.

jeromecoutant · 2021-06-24T09:25:55Z

We will take up resolving all the warnings in a separate issue.

ST_INTERNAL_REF 109607

0xc0170

Just one style nit

0xc0170 · 2021-06-24T14:43:34Z

targets/TARGET_STM/can_api.c

@@ -1130,6 +1135,8 @@ int can_filter(can_t *obj, uint32_t id, uint32_t mask, CANFormat format, int32_t
        {
            success = 1;
        }
+    } else if (format == CANAny) {
+	    success = 0;	// filter for CANAny is not supported by STM32, return a failure


aligned to 4 spaces (it's just 2?)

Corrected the alignment

Pull request has been modified.

0xc0170 · 2021-06-25T08:28:41Z

CI started

mbed-ci · 2021-06-25T08:47:14Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️

ciarmcom requested review from a team May 19, 2021 06:30

ciarmcom added needs: review devices: st labels May 19, 2021

0xc0170 reviewed May 21, 2021

View reviewed changes

MubeenHCLite mentioned this pull request May 25, 2021

STM32 CAN 'filter handle' operation and documentation incorrect #11953

Closed

cindyli-13 reviewed May 25, 2021

View reviewed changes

targets/TARGET_STM/can_api.c Outdated Show resolved Hide resolved

jeromecoutant approved these changes May 26, 2021

View reviewed changes

ciarmcom added the stale Stale Pull Request label Jun 1, 2021

mergify bot added needs: work and removed needs: review labels Jun 1, 2021

ciarmcom removed the stale Stale Pull Request label Jun 1, 2021

0xc0170 added the release-type: patch Indentifies a PR as containing just a patch label Jun 2, 2021

0xc0170 self-requested a review June 14, 2021 09:25

jeromecoutant suggested changes Jun 14, 2021

View reviewed changes

MubeenHCLite force-pushed the corrections_in_STM_canapi branch from 6b6cab7 to 4c47d15 Compare June 14, 2021 14:19

Updated comments appropriately

9732cdc

MubeenHCLite requested a review from jeromecoutant June 23, 2021 14:47

jeromecoutant approved these changes Jun 24, 2021

View reviewed changes

0xc0170 added needs: review release-type: patch Indentifies a PR as containing just a patch and removed needs: work labels Jun 24, 2021

0xc0170 previously requested changes Jun 24, 2021

View reviewed changes

mergify bot added needs: work and removed needs: review labels Jun 24, 2021

Adjusted spcae alignment

6f436db

MubeenHCLite requested review from jeromecoutant and 0xc0170 June 25, 2021 06:38

jeromecoutant approved these changes Jun 25, 2021

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Jun 25, 2021

0xc0170 approved these changes Jun 25, 2021

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jun 25, 2021

0xc0170 merged commit de7e326 into ARMmbed:master Jun 25, 2021

mergify bot removed the ready for merge label Jun 25, 2021

mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021

		@@ -956,8 +958,10 @@ int can_write(can_t *obj, CAN_Message msg, int cc)

		int can_read(can_t obj, CAN_Message msg, int handle)
		{

Changes required in STM CAN read API #14677

Changes required in STM CAN read API #14677

Uh oh!

Conversation

MubeenHCLite commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented May 19, 2021

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 May 21, 2021

Choose a reason for hiding this comment

Uh oh!

jeromecoutant Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 21, 2021

Uh oh!

adbridge commented May 24, 2021

Uh oh!

Uh oh!

ciarmcom commented Jun 1, 2021

Uh oh!

mergify bot commented Jun 1, 2021

Uh oh!

0xc0170 commented Jun 11, 2021

Uh oh!

0xc0170 commented Jun 14, 2021

Uh oh!

MubeenHCLite commented Jun 14, 2021

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

MubeenHCLite commented Jun 14, 2021

Uh oh!

jeromecoutant commented Jun 14, 2021

Uh oh!

mergify bot commented Jun 14, 2021

Uh oh!

cindyli-13 commented Jun 14, 2021

Uh oh!

MubeenHCLite commented Jun 15, 2021

Uh oh!

MubeenHCLite commented Jun 21, 2021

Uh oh!

pilotak commented Jun 23, 2021

Uh oh!

MubeenHCLite commented Jun 23, 2021

Uh oh!

jeromecoutant commented Jun 24, 2021

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jun 24, 2021

Choose a reason for hiding this comment

Uh oh!

MubeenHCLite Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 25, 2021

Uh oh!

mbed-ci commented Jun 25, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

Uh oh!

MubeenHCLite commented May 19, 2021 •

edited

Loading