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.

Sign up for GitHub

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

Jump to bottom

add ustring builtin type (unsigned char*) #125

Merged
merged 3 commits into from
Oct 5, 2020
Merged

add ustring builtin type (unsigned char*) #125

merged 3 commits into from
Oct 5, 2020

Conversation

pgu-swir
Copy link
Contributor

@pgu-swir pgu-swir commented Sep 17, 2020 •
edited
Loading

This answers issue #124 by using "ustring" and casting string types to (const char *) when used in the codecWrite.

This is still WIP as the server shim code needs as well to do some cast... but I wanted to know if the direction is satisfactory for you

@pgu-swir pgu-swir marked this pull request as draft September 17, 2020 08:12
@MichalPrincNXP
Copy link
Member

Hello @pgu-swir , I am sorry not being able to respond sooner when discussed with @Hadatko about this problem. To be honest, @Hadatko is more experienced in this field than me, however it seems to me OK to introduce ustring type and also the so-far-done steps should be ok. As for the failing Travis, please ignore this in both of your PRs ... I have migrated erpc project from travis-ci.org to travis-ci.com recently and there are build issues since that time (head of develop branch also fails under travis-ci.com while it was passing under travis-ci.org). Will try to solve this soon.

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Sep 18, 2020 •
edited
Loading

Hi @MichalPrincNXP , no worries. The "full" patch is available now and works for me and seems to have passed Travis tests :-)
I've tried to keep it as small as possible. For example, the xxx_local variables could have been as well "unsigned char*" but that does not bring a lot of benefits, so I left it as a char which is memcpy'd to the actual out function parameter

@pgu-swir pgu-swir marked this pull request as ready for review September 18, 2020 01:52
@pgu-swir pgu-swir changed the title add ustring for unsigned char and force cast to char* add ustring builtin type (unsigned char*) Sep 19, 2020
@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 5, 2020

Anything else I should do?

Hadatko
Hadatko approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@Hadatko Hadatko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

So far so good ;)

erpcgen/src/templates/c_coders.template
@@ -232,7 +232,7 @@ codec->readData({$info.name}, {$info.sizeTemp} * sizeof({$info.builtinTypeName})
{# Encode sending data #}
{% def encodeBuiltinType(info) ----------------- %}
{% if info.builtinType == "kStringType" %}
codec->writeString(strlen({$info.name}), {$info.name});
codec->writeString(strlen((const char*){$info.name}), (const char*){$info.name});
Copy link
Member

Choose a reason for hiding this comment

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

if info.builtinType == "kStringType" looks like it is not counting with ustring type. Am i wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Ok i looked into code and it should be ok.

MichalPrincNXP
MichalPrincNXP approved these changes Oct 5, 2020
View reviewed changes
Copy link
Member

@MichalPrincNXP MichalPrincNXP left a comment

Choose a reason for hiding this comment

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

Hello @pgu-swir , I have just finished testing on my side and all seems to be OK. Thanks for this effort!

@MichalPrincNXP MichalPrincNXP merged commit 6cbc39c into EmbeddedRPC:develop Oct 5, 2020
@Hadatko
Copy link
Member

Hadatko commented Oct 5, 2020

@pgu-swir it would be good if you could add some erpcgen pytest. Not execution but for generating ouput. To keep support for future. Take a look here: https://github.com/EmbeddedRPC/erpc/tree/develop/erpcgen/test

@MichalPrincNXP
Copy link
Member

Well, Dusan, I have already updated tests + added one more test on my side ... I will provide this update on the develop branch later this week with other NXP changes for 1.8.0 version.

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 5, 2020

@pgu-swir it would be good if you could add some erpcgen pytest. Not execution but for generating ouput. To keep support for future. Take a look here: https://github.com/EmbeddedRPC/erpc/tree/develop/erpcgen/test

I'll look at it - I need to figure out exactly what/where to add something as ustring are really a string

@MichalPrincNXP
Copy link
Member

@pgu-swir it would be good if you could add some erpcgen pytest. Not execution but for generating ouput. To keep support for future. Take a look here: https://github.com/EmbeddedRPC/erpc/tree/develop/erpcgen/test

I'll look at it - I need to figure out exactly what/where to add something as ustring are really a string

Do not worry about this, I will manage that as written in my previous note.

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 5, 2020

@pgu-swir it would be good if you could add some erpcgen pytest. Not execution but for generating ouput. To keep support for future. Take a look here: https://github.com/EmbeddedRPC/erpc/tree/develop/erpcgen/test

I'll look at it - I need to figure out exactly what/where to add something as ustring are really a string

Do not worry about this, I will manage that as written in my previous note.

oh, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichalPrincNXP MichalPrincNXP MichalPrincNXP approved these changes

@Hadatko Hadatko Hadatko approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pgu-swir @MichalPrincNXP @Hadatko