Skip to content

Conversation

@alexanderwachter
Copy link
Member

@alexanderwachter alexanderwachter commented Nov 20, 2019

This commit moves the semaphore for a blocking call from
the driver implementation to the stack of the caller.
It reduces the complexity of the drivers and saves some RAM.

Fixes #22379

This commit moves the semaphore for a blocking call from
the driver implementation to the stack of the caller.
It reduces the complexity of the drivers and saves some RAM.

Signed-off-by: Alexander Wachter <alexander@wachter.cloud>
@alexanderwachter alexanderwachter added RFC Request For Comments: want input from the community area: CAN labels Nov 20, 2019
@alexanderwachter alexanderwachter added this to the v2.2.0 milestone Nov 20, 2019
@zephyrbot zephyrbot added the area: API Changes to public APIs label Nov 20, 2019
@zephyrbot
Copy link

All checks passed.

checkpatch (informational only, not a failure)

-:321: WARNING:UNNECESSARY_ELSE: else is not generally useful after a break or return
#321: FILE: include/drivers/can.h:368:
+		return api->send(dev, msg, timeout, callback_isr, callback_arg);
+	} else {

- total: 0 errors, 1 warnings, 288 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

struct can_send_wait send_wait;
int ret;

k_sem_init(&send_wait.sem, 0, 1);
Copy link
Member

Choose a reason for hiding this comment

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

This change will introduce an overhead of initialising a semaphore on each can_send() call without any callback defined. I am not sure it is worth it.

It also seem very unlike any other API implementation in Zephyr?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I am sort of ambivalent about it. Not sure how much of a trade-off the memory saved vs the calling overhead it is.
In terms of driver complexity I think it isn't a big improvement, we all had the same approach and it's not so hard to follow either.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change will introduce an overhead of initialising a semaphore on each can_send() call without any callback defined. I am not sure it is worth it.

The overhead of initializing is not that much. The blocking send call is used rarely, and in the current implementation, we need to maintain a sem for every mailbox just in case we will need it.

In terms of driver complexity I think it isn't a big improvement, we all had the same approach and it's not so hard to follow either.

The complexity raised with the timeout patch :-)
I really don't like #19707 in its current state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha. What would be your plan then, use that can_common_isr_callback_handler() for failing sends with timeouts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated #19707 on top of this PR. Looks better now!

@galak galak removed this from the v2.2.0 milestone Feb 12, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 29, 2020
@nashif nashif added the Stale label Sep 7, 2020
@github-actions github-actions bot closed this Sep 22, 2020
@alexanderwachter alexanderwachter deleted the can_blocking_cleanup branch January 30, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: CAN has-conflicts Issue/PR has conflicts with another issue/PR platform: NXP NXP platform: STM32 ST Micro STM32 RFC Request For Comments: want input from the community Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

stm32: can_send gets stuck (ignoring timeout) if bus not present

6 participants