Updating MQTT client example for CryptoAuthLib #32
Conversation
…ce-sdk-embedded-c into cryptoauthlib_example
atigyi
left a comment
atigyi
left a comment
There was a problem hiding this comment.
Looks good, some comments though.
| const char* iotc_publish_message; | ||
| const char* iotc_private_key_filename; | ||
|
|
||
| #ifdef ENABLE_CRYPTOAUTHLIB |
There was a problem hiding this comment.
So if cryptoauthlib is enabled, then private_key_filename is not necessary. So we could go with the ugly:
#ifdef XXX
#else
#endif
pattern with all places.
There was a problem hiding this comment.
Why don't we handle this with slack? Use all variables and all commandline arguments for each configuration. Although this won't be full correct: since one could ask "Why do you allow to pass both a private key filename and a slotid at the same time?" The answer is because of code simplicity. And we trust our users know what they do.
There was a problem hiding this comment.
If we go with the slack version then most of the #ifdef can be removed easing the reader's job, keeping code simpler, easier to maintain.
There was a problem hiding this comment.
e.g. all #ifdefs can go away in this file if we decide so.
| * | ||
| * Used to sign tokens using the private key stored in a secure element. | ||
| */ | ||
| int init_cryptoauthlib(); |
There was a problem hiding this comment.
if you'd call this init_example_dependencies (+1 abstraction layer and not being specific) then 2 more #ifdefs could be removed. One here and one in the example since the example could call it all the way.
| For more information, please see the iotc_crypto_key_data_t | ||
| documentation in include/iotc_types.h. */ | ||
|
|
||
| #ifndef ENABLE_CRYPTOAUTHLIB |
There was a problem hiding this comment.
This can be removed too if we decide to branch at runtime: if we have private_key_filename, then load it, if there is no filename given, try to use the slotid (if available). If neither is available, then it is a parametrization issue, report an error message.
| int init_cryptoauthlib() { | ||
| ATCA_STATUS atca_status; | ||
| if ((atca_status = atcab_init(&cfg_ateccx08a_kithid_default)) != ATCA_SUCCESS) { | ||
| printf("Unable to initialize interface, status: %x\n", atca_status); |
There was a problem hiding this comment.
It seems the only #ifdef is required here (and the #include "cryptoauthlib.h")
| #include <portable.h> | ||
|
|
||
| void* iotc_bsp_mem_alloc(size_t byte_count) { | ||
| // workaround: FreeRTOS's pvPortMalloc crashes if 0 size |
There was a problem hiding this comment.
you should merge development->feature_cytpoauthlib->feature_cryptoauthlib_example to remove these
There was a problem hiding this comment.
pls do it since it makes review harder to fish out relevant changes, thx
There was a problem hiding this comment.
That's weird. I don't get it, especially since the PR is set to merge into feature_cytpoauthlib.
I must have messed up something, will figure this out tomorrow on my desktop.
…m/iot-device-sdk-embedded-c into feature_cryptoauthlib_example
* Bump Cryptoauthlib version to 20190304 * Enable new option to build lib as static
|
@fredrec would love to get this in for those using CryptoAuthLib, would you still like to merge this? I don't have much experience with it personally, just want to see if we can get some PRs cleaned up. |
|
Hi @fredrec would still like to merge this PR, if so can you please submit a commit so Travis CI could run it's tests, if not i'll close the PR. |
4 participants
No description provided.