Skip to content

[PR from CLI tool] Allow connectivity configs in MQTT demo to be CLI overridden #1124

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

Merged
merged 2 commits into from
Aug 13, 2020
Merged

[PR from CLI tool] Allow connectivity configs in MQTT demo to be CLI overridden #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3925ab5
Add #ifndefs around config macros in demos to allow them to be overri…
aggarw13 Aug 13, 2020
30e60b7
Add #error statements for BROKER_ENDPOINT in demo source files
aggarw13 Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions demos/mqtt/mqtt_demo_basic_tls/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,26 +56,30 @@
* the instructions in https://mosquitto.org/ for running a Mosquitto broker
* locally.
*/
#define BROKER_ENDPOINT "test.mosquitto.org"
#ifndef BROKER_ENDPOINT
Copy link
Contributor

@sarenameas sarenameas Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The philosophy of this file demo_config.h already is that applications can edit it.
In mqtt_demo_basic_tls.c we should have:

#ifndef BROKER_ENDPOINT
    #error "Please define an MQTT broker endpoint, BROKER_ENDPOINT in demo_config.h."
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, that our demo source files should have build-time failures for no definitions of these macros. Have added them in source files.

Copy link

Choose a reason for hiding this comment

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

So how would this build-time failures for no definitions of these macros work for our demo sources? Wont this file always define BROKER_ENDPOINT ? So by the time we get to our demo source files, wont the check for ifndef BROKER_ENDPOINT always say that BROKER_ENDPOINT is defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to guard against accidental removals of the BROKER_ENDPOINT by the customer. It provides a helpful message to remind them that the BROKER_ENDPOINT macro is required for the demo to run instead of giving them regular gcc build failures (at the source code points that use the macro)

Copy link
Member

Choose a reason for hiding this comment

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

This is still different from what the README says about Mutual Auth Demos.

#define BROKER_ENDPOINT "test.mosquitto.org"
#endif

/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief MQTT server port number.
*
* In general, port 8883 is for secured MQTT connections.
*/
#define BROKER_PORT ( 8883 )
#define BROKER_PORT ( 8883 )

/**
* @brief Path of the file containing the server's root CA certificate.
*
* This certificate should be PEM-encoded.
*/
#define ROOT_CA_CERT_PATH "certificates/mosquitto.org.crt"
#ifndef ROOT_CA_CERT_PATH
#define ROOT_CA_CERT_PATH "certificates/mosquitto.org.crt"
#endif

/**
* @brief Length of path to server certificate.
Expand Down
5 changes: 4 additions & 1 deletion demos/mqtt/mqtt_demo_basic_tls/mqtt_demo_basic_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,11 @@
* These configuration settings are required to run the basic TLS demo.
* Throw compilation error if the below configs are not defined.
*/
#ifndef BROKER_ENDPOINT
#error "Please define an MQTT broker endpoint, BROKER_ENDPOINT, in demo_config.h."
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this error ever trigger? Because we defined this macro in the config if it wasn't defined already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the customer happens to remove it from the config file, then this failure would remind them instead.

#endif
#ifndef ROOT_CA_CERT_PATH
#error "Please define path to Root CA certificate of the MQTT broker(ROOT_CA_CERT_PATH) in demo_config.h."
#error "Please define path to Root CA certificate of the MQTT broker, ROOT_CA_CERT_PATH, in demo_config.h."
#endif
#ifndef CLIENT_IDENTIFIER
#error "Please define a unique CLIENT_IDENTIFIER."
Expand Down
4 changes: 3 additions & 1 deletion demos/mqtt/mqtt_demo_lightweight/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@
* This demo uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
*/
#define BROKER_ENDPOINT "test.mosquitto.org"
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
Copy link
Contributor

Choose a reason for hiding this comment

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

The cautionary language on lines 53-55 is not sufficient here. From the test.mosquitto.org documentation, the test.mosquitto.org broker "...will often be running unreleased or experimental code and may not be as stable as you might hope.". Additional cautionary language on their website asks "...please do not abuse or rely upon it for anything of importance.".

I would recommend removing references to test.mosquitto.org from this (and any other) MQTT demos. If someone wishes to use it, they of course may do so. However the best way to test this by running a broker locally, and we can easily provide instructions for setting up mosquitto or another broker to run locally.

Copy link
Member

Choose a reason for hiding this comment

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

Such directions are available in the readme too.

Copy link

Choose a reason for hiding this comment

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

+1 to Gary's comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good suggestion to not suggest test.mosquitto.org in our demos. I had added instructions on the REAMDE for a way to setup the Mosquitto broker locally: https://github.com/aws/aws-iot-device-sdk-embedded-C/tree/development#installing-mosquitto-to-run-mqtt-demos-locally

We could update the comments on our demo_config.h to point to these instructions

#endif

/**
* @brief Length of MQTT server host name.
Expand Down
5 changes: 5 additions & 0 deletions demos/mqtt/mqtt_demo_lightweight/mqtt_demo_lightweight.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@
/* Reconnect parameters. */
#include "transport_reconnect.h"

/* Check that the broker endpoint is defined. */
#ifndef BROKER_ENDPOINT
#error "Please define an MQTT broker endpoint, BROKER_ENDPOINT, in demo_config.h."
#endif

/* Check that client identifier is defined. */
#ifndef CLIENT_IDENTIFIER
#error "Please define a unique CLIENT_IDENTIFIER."
Expand Down
4 changes: 3 additions & 1 deletion demos/mqtt/mqtt_demo_plaintext/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@
* This demo uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
*/
#define BROKER_ENDPOINT "test.mosquitto.org"
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
#endif

/**
* @brief MQTT server port number.
Expand Down
5 changes: 4 additions & 1 deletion demos/mqtt/mqtt_demo_plaintext/mqtt_demo_plaintext.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@
* These configuration settings are required to run the plaintext demo.
* Throw compilation error if the below configs are not defined.
*/
#ifndef BROKER_ENDPOINT
#error "Please define an MQTT broker endpoint, BROKER_ENDPOINT, in demo_config.h."
#endif
#ifndef CLIENT_IDENTIFIER
#error "Please define a unique CLIENT_IDENTIFIER."
#error "Please define a unique CLIENT_IDENTIFIER in demo_config.h."
#endif

/**
Expand Down