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

Adds an NFC System Test suite #9184

Merged
merged 14 commits into from
Feb 11, 2019
Merged

Adds an NFC System Test suite #9184

merged 14 commits into from
Feb 11, 2019

Conversation

ConradBraam
Copy link
Contributor

@ConradBraam ConradBraam commented Dec 21, 2018

Description

A icetea test framework based test suite, requires a card reader attached to the host and a target which is NFC capable using one of the supported drivers+shield.
see: readme file /TEST_APPS/testcases/nfc/README.MD includes notes to make test work with customer supplied NFC drivers, and memory limitations. Test suite only supports Type4 tag emulation EEPROM/Controller chips.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

Release Notes

Adds an automated test suite which provides Standalone and End-2-End validation for either the supported PN512 NFC Controller shield or the M24SR NFC EEPROM. This is an icetea framework based test suite and requires additional hardware.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 21, 2018

Please review Travis astyle job, there are failures.

I assume once reviewers are done, this will be rebased (history needs clean up - some merges can be eliminated as well)

@ciarmcom ciarmcom requested review from a team December 21, 2018 16:00
@ciarmcom
Copy link
Member

@ConradBraam, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-pan @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-test @ARMmbed/mbed-os-tls @ARMmbed/mbed-os-maintainers please review.

@cmonr
Copy link
Contributor

cmonr commented Dec 21, 2018

@ConradBraam Looks like the review-bot found an interesting issue with the PR.

This PR has a merge commit from ARMmbed/master in it, which confused the bot. Please remove the commit, and instead rebase your master branch to update this PR.

@cmonr cmonr removed request for a team December 21, 2018 16:05
@cmonr
Copy link
Contributor

cmonr commented Dec 21, 2018

@ConradBraam Taking a closer look at this commit, it looks like multiple merges from ARMmbed/master into this PR branch occurred, which will make the resolution much more difficult.

I would suggest taking a look at interactive rebasing the branch (git rebase -i) for more information.

@geky @kjbracey-arm Y'all konw a bit more git-fu than I do. Any suggestions?

Copy link
Contributor

@OPpuolitaival OPpuolitaival left a comment

Choose a reason for hiding this comment

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

I will send information in email

@@ -0,0 +1,284 @@
/* mbed Microcontroller Library

Choose a reason for hiding this comment

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

smart poster class is only for testing? shouldn't it be available for normal usage?

Copy link
Member

Choose a reason for hiding this comment

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

That would be useful to have a Smart Poster class in the codebase however we should do design review of the example one and see if it can fit the mbed os codebase.

using mbed::nfc::ndef::common::span_from_cstr;


//void cmd_ready_cb(int retcode)

Choose a reason for hiding this comment

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

leftover debug?

}

const char *errorcodes = // descriptions from nfc/stack/nfc_errors.h
" 0 NFC_OK \n"

Choose a reason for hiding this comment

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

does \n without \r work as a newline?

#include "nfcprocess.h"
#include "nfccommands.h"

events::EventQueue nfcQueue;

Choose a reason for hiding this comment

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

are these meant to be global?
you also mix naming convetions, camel, snake (and hungarian notation?)

Copy link
Contributor

Choose a reason for hiding this comment

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

#if MBED_CONF_NFCEEPROM
mbed::nfc::NFCEEPROMDriver& eeprom_driver = get_eeprom_driver(nfcQueue);

return ( (NFCTestShim *)(new NFCProcessEEPROM(nfcQueue, eeprom_driver)) );

Choose a reason for hiding this comment

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

it's OK, you don't need to cast

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

}

int HandleTestCommand::cmd_read_message(int argc, char *argv[]) {
nfcQueue.call(pNFC_Test_Shim, &NFCTestShim::on_read_nfceeprom);

Choose a reason for hiding this comment

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

the "on_" prefix is confusing as this isn't an event triggered by reading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this to follow the cmd_ prefix which makes the invoke source clearer and is consistent

return(CMDLINE_RETCODE_INVALID_PARAMETERS);
} else {
// parse arg and queue it up
char * uri = (char*)malloc(MAX_URL_LENGTH+1);

Choose a reason for hiding this comment

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

you can malloc and copy the correct length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, cleaner to copy just the string.


// todo: jira IOTPAN-295
int HandleTestCommand::cmd_erase(int argc, char *argv[]){
nfcQueue.call(pNFC_Test_Shim, &NFCTestShim::on_erase);

Choose a reason for hiding this comment

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

same problem with on_ prefix (unless I misunderstood the flow?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cmd_ prefixed now

@pan-
Copy link
Member

pan- commented Feb 6, 2019

@cmonr Git magic applied.
@paul-szczepanek-arm any thing left ?

@ConradBraam
Copy link
Contributor Author

@0xc0170 Anything still left to cover?

@0xc0170 0xc0170 dismissed their stale review February 7, 2019 08:52

will rereview shortly

0xc0170
0xc0170 previously requested changes Feb 7, 2019
.gitignore Outdated Show resolved Hide resolved
TEST_APPS/device/nfcapp/NFCCommands.cpp Outdated Show resolved Hide resolved
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 7, 2019

This test addition deserves release notes addition ! please add it to the first comment above (following ARMmbed/mbed-os-5-docs#933)

@NirSonnenschein
Copy link
Contributor

starting CI

@mbed-ci
Copy link

mbed-ci commented Feb 10, 2019

Test run: SUCCESS

Summary: 9 of 9 test jobs passed
Build number : 1
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 11, 2019

@AnotherButler Can you please review Readme addition here?

Waiting for the last review from docs, and this will be ready for integration

@0xc0170 0xc0170 merged commit 830c3ec into ARMmbed:master Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.