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

[FEATURE] Added SDO client functionality #84

Merged
merged 18 commits into from
Mar 22, 2022

Conversation

dozack
Copy link
Contributor

@dozack dozack commented Mar 1, 2022

Sorry i opened merge request into wrong branch. This is correct one.

Motivation

see #82

Definition of done

  • specify API allowing user to request SDO client transfer
  • minimal implementation allowing SDO expedited transfer protocol [CiA 301]

Description

Suggested API is based on direct transfer initiation (request is composed and transmitted during user API call) and asynchronous response. For each API call, user provides request information (index, subindex) and custom callback which is executed after transfer is completed, aborted or times out.

co_csdo_uml

Conclussion

Currently for my project, i only require expedited transfer functionality, which i have almost figured out (if we agree on API specification). I will not have time for segmented and block transfer implementation before May/June this year. After my finals i am willing to contribute more if it will not be done yet.

Thank you for your interest in my contributions and looking forward for feedback.

@dozack dozack changed the title Feature sdo client [FEATURE] Added SDO client Mar 1, 2022
@dozack dozack changed the title [FEATURE] Added SDO client [FEATURE] Added SDO client functionality Mar 1, 2022
@dozack
Copy link
Contributor Author

dozack commented Mar 1, 2022

Update

Progress on SDO client implementation by me can be seen on feat-sdo-client-impl branch. I switched to another branch in case you disagree with API specification of mine. If my concept will be accepted, i will merge the mentioned branch into one related to this pull request.

@michael-hillmann
Copy link
Contributor

I think this API is good and flexible for many use-cases. When we support expedited transfer in a first step, it is absolutely ok. The interface you propose could be extended to the segmented and block transfers. In this case we use an internal callback, which handles each single message and collects the data until the whole block is finished. At the end, the user callback is called for using the provided data or error management.

@michael-hillmann michael-hillmann added the enhancement New feature or request label Mar 1, 2022
@michael-hillmann michael-hillmann added this to the 4.2.0 milestone Mar 1, 2022
@dozack
Copy link
Contributor Author

dozack commented Mar 1, 2022

Thanks for response, i plan on using CO_SDO_BUF type for storing tranfered data temporarily and user can copy them out in callback.

@michael-hillmann
Copy link
Contributor

michael-hillmann commented Mar 2, 2022

Hmm, let's see. We must be carefully when using the SDO server memory, because the server could be active during the same time we start the SDO client transfers.

Maybe the user provide a transfer buffer (with size) via the API function?

@dozack
Copy link
Contributor Author

dozack commented Mar 3, 2022

I have added new SDO buffer explicitly for client into CO_NODE and CO_NODE_SPEC structures. If SDO client feature will be disabled, user can pass NULL reference to SDO client buffer in CO_NODE_SPEC. This way client and server instance memory is completely decoupled.

Copy link
Contributor

@michael-hillmann michael-hillmann left a comment

Choose a reason for hiding this comment

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

Looks good for me. Unfortunately, this is a breaking change for the existing users. I see no way to avoid this ;-<.
Note: We are a step before a major update anyway (4.1.x to 4.2.x). So we just need to add this to the list of breaking changes.

@michael-hillmann
Copy link
Contributor

Hi, what do you think on the following idea:

// API function as you defined it
CO_ERR COCSdoRequestDownload(CO_CSDO *csdo,
                             uint16_t index,
                             uint8_t sub,
                             void *value,   // <<< maybe change this to: uint8_t *buffer
                             uint32_t size, // <<< number of bytes
                             CO_CSDO_DN_CALLBACK_T callback,
                             uint32_t timeout);

// we could change the upload API function analog to download with memory for transfer:
CO_ERR COCSdoRequestUpload(CO_CSDO *csdo,
                           uint16_t index,
                           uint8_t sub,
                           uint8_t *buffer,   // <<< transfer buffer memory start
                           uint32_t size,     // <<< size of transfer buffer
                           CO_CSDO_DN_CALLBACK_T callback,
                           uint32_t timeout);

With this small change, the user will give us the transfer buffer memory for the client transfer. So we don't need to allocate any memory within the CANopen stack library.

@dozack
Copy link
Contributor Author

dozack commented Mar 22, 2022

I have finished partial implementation of client allowing expedited transfer. Currently it has been tested against CANoe simulation. I am currently working on deploying my application on real bus with motor controllers.

Copy link
Contributor

@michael-hillmann michael-hillmann left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for this big step to a more complete CANopen stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants