-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
samples: adding sample to communicate with aws iot #23469
Conversation
Signed-off-by: Mohammad Jamal Mohiuddin <md.jamalmohiuddin@gmail.com>
Some checks failed. Please fix and resubmit. checkpatch issues
Kconfig issues
Nits issues
Gitlint issues1: UC6 Body has no content, should at least have 1 line. Identity/Emails issues5c0c394: author email (David Developer md.jamalmohiuddin@gmail.com) needs to match one of the signed-off-by entries. Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the sample! Looks good, just minor changes needed.
@@ -0,0 +1,9 @@ | |||
# SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the commit subject to samples: net: cloud: Add AWS IoT sample
You also need to add something in commit body. In this case it could just be something like
Adding a network sample to communicate with Amazon cloud.
Please also go through style issues that the checkpatch complains about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do i need to push a new commit or create a new pull request, i am unaware about the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just squash your changes together (as you had only 1 commit in this PR) and then force push to this branch related to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to confirm, make all the changes in another commit and push to the branch,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to confirm, make all the changes in another commit and push to the branch,
no, just squash the changes in the same branch and force push to this PR
CONFIG_MAIN_STACK_SIZE=4096 | ||
|
||
# Enable Logging support | ||
CONFIG_LOG_IMMEDIATE=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the immediate logging, it can slow down the system and is useful only in limited use cases.
CONFIG_NET_CONNECTION_MANAGER_LOG_LEVEL_DBG=n | ||
CONFIG_DNS_RESOLVER_LOG_LEVEL_DBG=n | ||
CONFIG_NET_CONTEXT_LOG_LEVEL_DBG=n | ||
CONFIG_SOC_PART_NUMBER_SAME70Q21B=y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove board specific stuff from this file. If you need to enable something board specific, create boards
directory and put board file there (see example in various networking samples what this means).
tests: | ||
sample.net.cloud.aws_mqtt: | ||
harness: net | ||
platform_whitelist: sam_e70_xplained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the whitelisting and put instead minimum ram requirement. That way the sample can be run any device with network connectivity and enough ram.
|
||
#error "Fill this before generating the build" | ||
|
||
unsigned char amazon_certificate[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that we do things here similar way as in other samples where some file needs to be embedded into the binary. So please write this as
unsigned char amazon_certificate[] = {
#include "amazon.cert"
}
if the cert is in ascii format. If you have the cert in binary format (DER), there is automatic way to create a file that can be included here (search generate_inc_file in networking samples for an example).
The point here is that the amazon.cert file is not put to version control and this will not compile if user does not provide the file. Some nice error message can be generated of course.
Edit: we probably need to provide some sample cert files so that the sample compiles, otherwise we cannot build test this. The certs do not need to have proper credentials of course.
#define ALIVE_TIME (MSEC_PER_SEC * 60U) | ||
#define APP_MQTT_BUFFER_SIZE 128 | ||
#error "Fill the Host Name and compile" | ||
#define CONFIG_AWS_HOSTNAME "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hostname must be provided via Kconfig file in this case. A sample app can have a Kconfig file of its own, see other networking samples how to do it.
Probably also port and mqtt client id should be put to kconfig file too as these can change for different users.
|
||
net_ipaddr_copy(&broker4->sin_addr, | ||
&net_sin(haddr->ai_addr)->sin_addr); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
|
||
LOG_INF("PUBREC packet id: %u", evt->param.pubrec.message_id); | ||
|
||
const struct mqtt_pubrel_param rel_param = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not introduce variables in the middle of the block but place them at the start instead.
|
||
LOG_INF("PUBCOMP packet id: %u", | ||
evt->param.pubcomp.message_id); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra empty line here.
char topic[] = "/things/"; | ||
u8_t len = strlen(topic); | ||
struct mqtt_publish_param param; | ||
char pub_msg[64]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation wrong. Also we try to avoid magic constants, please use defines instead for the length
}; | ||
|
||
|
||
unsigned char private_key[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this key expected to be filled in by someone? Please look at the samples/net/cloud/google_iot_mqtt
to see how the keys are handled there. Right now, it isn't obvious how this would be loaded (or that it needs to be).
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Signed-off-by: Mohammad Jamal Mohiuddin md.jamalmohiuddin@gmail.com