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

Create a CMake macro for defining aliases for configuration parameters #1596

Merged
merged 16 commits into from
Mar 19, 2021

Conversation

yourslab
Copy link
Contributor

@yourslab yourslab commented Mar 16, 2021

This change allows aliases to be defined for macros by defining a macro called set_alias.

This is done to fix issue #1583, allowing THING_NAME to be an alias for CLIENT_IDENTIFIER.

It also fixes issue #1584 so that the values of any parameters or the file from which they are retrieved are logged. Here's an example of the log output:

Using values in test_config.h to define the following macros for http_system_test:
ROOT_CA_CERT_PATH
SERVER_HOST
HTTPS_PORT
Found the following definitions for http_system_test: ROOT_CA_CERT_PATH, SERVER_HOST, HTTPS_PORT
All required definitions for http_system_test were found - Adding to default target.

Using values in test_config.h to define the following macros for mqtt_system_test:
BROKER_PORT
Using the passed CMake arguments to define the following macros for mqtt_system_test:
CLIENT_IDENTIFIER = three
BROKER_ENDPOINT = foo
Found the following definitions for mqtt_system_test: CLIENT_IDENTIFIER, BROKER_ENDPOINT, BROKER_PORT
WARNING: mqtt_system_test missing definitions for macros: ROOT_CA_CERT_PATH, CLIENT_CERT_PATH, CLIENT_PRIVATE_KEY_PATH
To run mqtt_system_test, define required macros in mqtt_system_test/demo_config.h.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

tools/cmake/utility.cmake Outdated Show resolved Hide resolved
@@ -42,9 +42,9 @@ set_macro_definitions(TARGETS ${DEMO_NAME}
"ROOT_CA_CERT_PATH"
"CLIENT_CERT_PATH"
"CLIENT_PRIVATE_KEY_PATH"
"CLIENT_IDENTIFIER"
"THING_NAME"
Copy link
Contributor

@aggarw13 aggarw13 Mar 16, 2021

Choose a reason for hiding this comment

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

As part of best practices for connection with IoT Core, the Client ID should be the same as the Thing Name.
So, should we make the CLIENT_IDENTIFIER as the required config but THING_NAME as the optional config?

Copy link
Contributor Author

@yourslab yourslab Mar 17, 2021

Choose a reason for hiding this comment

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

Required in the context of this function means that a definition is required for it to compile without error. In that case, I think both of these macros should be required. In fact, nothing in the defender demo should be optional.

I've updated the docstring of set_macro_definitions to specify this.

CHANGELOG.md Show resolved Hide resolved
aggarw13
aggarw13 previously approved these changes Mar 17, 2021
@muneebahmed10
Copy link
Contributor

The demo_config.h for Shadow defines both a client identifier:

#define CLIENT_IDENTIFIER "testclient"
and a thing name: Do these need to be changed?

@yourslab
Copy link
Contributor Author

yourslab commented Mar 18, 2021

Perhaps, it would be best practice to set CLIENT_IDENTIFIER to THING_NAME in the config. The CMake only makes it so that it does that when passed as a CLI argument.

aggarw13
aggarw13 previously approved these changes Mar 18, 2021
tools/cmake/utility.cmake Outdated Show resolved Hide resolved
tools/cmake/utility.cmake Outdated Show resolved Hide resolved
yourslab and others added 3 commits March 18, 2021 11:01
Co-authored-by: Archit Aggarwal <architag@amazon.com>
Co-authored-by: Archit Aggarwal <architag@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants