Skip to content

Conversation

@nordic-krch
Copy link
Contributor

@nordic-krch nordic-krch commented Feb 3, 2020

A draft showing how to handle external API with devices.

Short summary:

  • device structure gets additional field - a pointer to extension api structure which contains mask of capabilities and pointer to data. if more than one extension is supported then data is an array of pointers associated with each api.
    struct device_ext_api *ext_api;
  • device.h gets internal api for checking if device supports given extension (z_device_ext_api_supported(dev, bitmask)) and getting data associated with that extension (z_device_ext_api_get_data(dev, bitmask))
    static inline bool z_device_ext_api_supported(struct device *dev, u32_t api_mask)
  • external api (onoff service in that example) implements simple inline like device_get_onoff_service(dev, &srv)
    static inline int device_get_onoff_service(struct device *dev,

Example:

err = device_subsys_get_onoff_service(clock, subsys, &srv);
if (err < 0) {
    LOG_ERR("Failed to get device onoff service (err: %d)", err);
}

err = onoff_request(srv, &client);

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

A draft showing how to handle external API with devices.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@nordic-krch nordic-krch added RFC Request For Comments: want input from the community area: API Changes to public APIs DNM This PR should not be merged (Do Not Merge) labels Feb 3, 2020
@zephyrbot zephyrbot added the area: Samples Samples label Feb 3, 2020
@zephyrbot
Copy link

All checks passed.

checkpatch (informational only, not a failure)

-:62: WARNING:LONG_LINE: line over 80 characters
#62: FILE: include/device.h:328:
+static inline bool z_device_ext_api_supported(struct device *dev, u32_t api_mask)

-:147: WARNING:LINE_SPACING: Missing a blank line after declarations
#147: FILE: include/sys/onoff.h:462:
+	void **data;
+	if (!z_device_ext_api_supported(dev, DEVICE_EXT_API_ONOFF)) {

- total: 0 errors, 2 warnings, 176 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.

@tbursztyka
Copy link
Contributor

As for spi and i2c, you are pushing a specialized feature (or if you prefer: a "not-generic enough one") to a generic node. Imo, your are taking the problem the wrong way. You are not responding to what we have been waiting for: a transaction manager RFC. If you do that you'll see what belongs to the transaction manager and what may belong to something else below.

@nordic-krch
Copy link
Contributor Author

nordic-krch commented Feb 3, 2020

@tbursztyka we already have similar case. There is an onoff service and clock_control driver. I would like to use onoff service on clock device and not extended clock control API. How to do that? Maybe you could propose something as you clearly have something in mind and it is different than this proposal?

Transaction manager is another case. We can have RFC with API for that but still there will be an issue how to use it with the device.

@pabigot
Copy link
Contributor

pabigot commented Feb 3, 2020

Could we please start developing a consensus on what we need before we start proposing technical solutions that have to be reviewed without anything to judge them against?

Also: https://docs.zephyrproject.org/latest/reference/drivers/index.html#device-specific-api-extensions

which might be extended to subsystem-specific-api-extensions if that seems like a good solution for whatever the problem is.

@nordic-krch
Copy link
Contributor Author

@pabigot issue created #22415

/** @brief Check if given extension is supported.
*
* @param dev Device.
* @param api_mask Bit mask identifiing an extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use API ID number instead. 32 possibilities are not so much. Independently form fact how compatibility information is stored in drivers properties record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes api id would allow extension beyond 32 bits and if 32 is too low at some point it can be internally extended without change of any API. I would like to keep it as bitmask internally to reduce overhead. Currently with device supporting only one api extension it is almost instant. Question is if we foresee devices with multple extensions (probably yes).

@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 30, 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 Jul 2, 2020
@pabigot
Copy link
Contributor

pabigot commented Jul 7, 2020

API 2020-07-07: Moved from triage to in-progress; relates to #23371.

@nashif nashif removed the has-conflicts Issue/PR has conflicts with another issue/PR label Jul 7, 2020
@nashif nashif added the Stale label Sep 7, 2020
@github-actions github-actions bot closed this Sep 22, 2020
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: Samples Samples DNM This PR should not be merged (Do Not Merge) RFC Request For Comments: want input from the community Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants