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

Add C API for Greentea client #4364

Merged
merged 6 commits into from Jun 19, 2017

Conversation

Projects
None yet
9 participants
@mazimkhan
Contributor

mazimkhan commented May 22, 2017

Description

mbedtls unit testing framework is enhanced to execute on target. PR ARMmbed/mbedtls#930
Since the mbed-tls is a C library and its unit test framework is also written in C. In order to run it with Greentea we need C API for the Greentea client in mbed-os. This API adds extern "C" wrappers for C linkage of Greentea Client API needed by mbedtls unit test.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Related PRs

PR in mbedtls ARMmbed/mbedtls#930

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 22, 2017

@mazimkhan Could you fill out the relevant sections of the template and remove the rest?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 22, 2017

@mazimkhan Could you change the title to match the guidelines for a good commit message? In particular, could you update it to be in the imperative mood? https://chris.beams.io/posts/git-commit/ We use PR titles in the release notes.

@mazimkhan mazimkhan changed the title from Mbed tls test to Add C API for Greentea client May 22, 2017

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented May 22, 2017

@theotherjimmy Thanks for pointing this out.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented May 22, 2017

@sg- @bridadan @adbridge @0xc0170 Please review and merge.

@sg-

Why are new functions created rather than adding extern "C" to the existing greentea functions? Given the implementation is still in .cpp I seems that is what you meant todo?

@sg- sg- added the needs: review label May 22, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented May 22, 2017

Agreed with @sg-, can the existing function for which you've added the c wrappers just be moved to a .c file? Seems like that should be non breaking change (could be missing something though). I'd rather avoid creating wrappers for functions.

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 23, 2017

Inline with the above 2 comments . '_c' function names are pretty horrible ...

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented May 23, 2017

@sg- @bridadan I understand what you mean. But since I had to create a new test_env_c.h for including in my C test code I thought of keeping everything completely separate. test_env.h has polymorphic functions named greentea_send_kv that prevent inclusion in C code.

But I have consolidate it together to use the same functions. Please review updated code.

features/frameworks/greentea-client/greentea-client/test_env.h Outdated
@@ -28,6 +28,9 @@
#endif
#include <stdio.h>
extern "C" {

This comment has been minimized.

@pan-

pan- May 23, 2017

Member

Why not put these functions into this header and exclude C++ parts:

// C declaration block
#ifdef __cplusplus
 extern "C" {
#endif

void GREENTEA_SETUP(const int timeout, const char * host_test);
void greentea_send_kv(const char * key, const char * val);
int greentea_parse_kv(char * key, char * val,
                        const int key_len, const int val_len);

#ifdef __cplusplus
}
#endif

// C++ declaration block
#ifdef __cplusplus
// put C++ functions here
#endif

This comment has been minimized.

@mazimkhan

mazimkhan May 23, 2017

Contributor

isn't it cleaner without lots of ifdefs?

This comment has been minimized.

@pan-

pan- May 23, 2017

Member

Unfortunately there is trade of to make, I don't believe we will get any thing very clean out of this.

Splitting definitions into separate file does not reduce the number of ifdef in the code or if it does it is at the expense of user friendliness.

First of all consider that if the proposed solution is chosen then declaration in test_env_c.h shall be guarded with

#ifdef __cplusplus
 extern "C" {
#endif

// declaration 

#ifdef __cplusplus
}
#endif

Otherwise if ever a C++ file include test_env_c.h the functions declared will be seen as C++ symbols. Note that the inclusion guard in test_env.h can be removed.

Secondly, to allow inclusion of test_env.h within C code then C++ bits have to be guarded.

None of those guards are mandatory but they improve the end user experience.

This comment has been minimized.

@mazimkhan

mazimkhan May 23, 2017

Contributor

OK. Actually that's the point of splitting into two files. The test_env.h is included only by C++ code hence does not need ifdef __cplusplus. Similarly test_enc_c.h is only included in the C code (as suggested by '_c') hence no need to put extern "C" there.

This comment has been minimized.

@pan-

pan- May 23, 2017

Member

@sg- If a module expose a C and a C++ API what is the mbed way of exposing its APIs ?

This comment has been minimized.

@sg-

sg- May 23, 2017

Member

This hasn't really come up before. There has always been clear separation between C and C++ code. Question really is why this isn't the case now for greentea? I'd guess this has to do with using RawSerial and Tickers in the test harness?? @mazimkhan are you planning to bind greentea to mbed OS drivers or a different runtime for put/get?

This comment has been minimized.

@mazimkhan

mazimkhan May 24, 2017

Contributor

I would prefer a standard runtime of getc/putc. But I don't intend to change existing implementation here. I need Greentea API for the mbedtls C test code.
Are you suggesting C implementation of Greentea API. It seems reasonable since it has no C++.

This comment has been minimized.

@0xc0170

0xc0170 May 24, 2017

Member

As @pan- suggested, one header file and make it compatible for both languages. Thats the entry point for tests (test_env.h). C++ can use both provided API, C only C part of it.

This comment has been minimized.

@mazimkhan

mazimkhan May 26, 2017

Contributor

Ok if that is the practice. But we are making this header common for C and C++.
For any odd reason we would want to switch to using .hpp or .hh extension for C++ headers than it would look odd.

I will update the code based on the suggestion.

features/frameworks/greentea-client/source/greentea_test_env.cpp Outdated
@@ -532,7 +531,7 @@ enum Token {
* \return Next character from the stream or EOF if stream has ended.
*
*/
static int _get_char() {
extern "C" char greentea_getc() {

This comment has been minimized.

@pan-

pan- May 23, 2017

Member
  • May it be in greentea_serial.cpp ?
  • Is there a reason to not return an int like the standard getc function ?

This comment has been minimized.

@mazimkhan

mazimkhan May 23, 2017

Contributor

May it be in greentea_serial.cpp ?

May be but greentea_serial.cpp looks more like creation of the singleton greentea_serial object. Most of the API ('greentea_') implementation is in greentea_test_env.cpp. I just changed the name.

Is there a reason to not return an int like the standard getc function ?

No. reverted to int. Please review updated code.

This comment has been minimized.

@pan-

pan- May 23, 2017

Member

May be but greentea_serial.cpp looks more like creation of the singleton greentea_serial object. Most of the API ('greentea_') implementation is in greentea_test_env.cpp. I just changed the name.

That is a good point and in some way greentea_getc is the counter part of greentea_send_kv which is declared in test_env.h.

features/frameworks/greentea-client/greentea-client/test_env_c.h Outdated
void greentea_send_kv(const char * key, const char * val);
int greentea_parse_kv(char * key, char * val,
const int key_len, const int val_len);
char greentea_getc();

This comment has been minimized.

@pan-

pan- May 23, 2017

Member

This function belongs to greentea_serial.h module.
C and C++ parts can be isolated as shown above.

This comment has been minimized.

@mazimkhan

mazimkhan May 23, 2017

Contributor

isn't it cleaner without lots of ifdefs?

@sg-

This comment has been minimized.

Member

sg- commented Jun 3, 2017

@pan- approve the updates?

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Jun 5, 2017

@pan- @sg- Thanks. I am yet to make the updates. I will ping you once done.

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Jun 5, 2017

@pan- please review updated code.

@pan-

pan- approved these changes Jun 5, 2017

LGTM

@bridadan

Thanks for putting in the extra work @mazimkhan, this looks 1000x better! 👍

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Jun 5, 2017

Thanks @bridadan

@0xc0170

0xc0170 approved these changes Jun 5, 2017

It would be nice to have documentation for that public API but that is not the aim of this PR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 5, 2017

@mazimkhan Can you rebase this PR (that should resolve jenkins CI) ?

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Jun 8, 2017

Can we retest below CI and merge this:
continuous-integration/jenkins/pr-head

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 8, 2017

Jenkins CI restarted

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 8, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 8, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 8, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 502

All builds and test passed!

@tommikas

This comment has been minimized.

Contributor

tommikas commented Jun 9, 2017

jenkins/pr-head should pass if you rebase.

The failing build is currently disabled in master because the build doesn't work with it.

@mazimkhan mazimkhan force-pushed the mazimkhan:mbed-tls-test branch Jun 9, 2017

Mohammad Azim Khan and others added some commits Mar 29, 2017

@mazimkhan mazimkhan force-pushed the mazimkhan:mbed-tls-test branch to b3cbb07 Jun 12, 2017

@mazimkhan

This comment has been minimized.

Contributor

mazimkhan commented Jun 13, 2017

All tests pass and approvals received. Please merge it.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 13, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 13, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 544

All builds and test passed!

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 13, 2017

@theotherjimmy theotherjimmy merged commit 1ffbdfc into ARMmbed:master Jun 19, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment