-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Adding preprocessor checks before using DevelopmentCerts:kDac* constants #25467
Adding preprocessor checks before using DevelopmentCerts:kDac* constants #25467
Conversation
@@ -52,7 +52,11 @@ class ExampleDACProvider : public DeviceAttestationCredentialsProvider | |||
|
|||
CHIP_ERROR ExampleDACProvider::GetDeviceAttestationCert(MutableByteSpan & out_dac_buffer) | |||
{ | |||
#if 0x8000 <= CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID && CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID <= 0x801F |
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.
instead of hardcoding the 0x8000 here, can we define the CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID_MIN/MAX
or something in the ExampleDACs.h file?
It'd be nicer to have the control of the range specified close to where they're defined otherwise if we add additional product identifiers here you'd need to know to look at this spot too otherwise it won't work and you'd be wondering why
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.
Actually, wouldn't it be better to simply add a GN switch not to compile ExampleDACProvider
if you use a non-default product ID? This class is only for test purposes so it shouldn't be used if you have your own DAC. Btw, why is the linker error occuring? Is ExampleDACProvider
instantiated in your application? Wouldn't removing ExampleDACProvider
from the application be enough to resolve it?
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.
Done, in this PR: #25953
Closing this one.
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.
It seems like this should be some GN compile flag deciding if example is compiled in or not. I.e. compilation should just not try to compile in instead of getting some link error.
Done, in this PR: #25953 Closing this one. |
Fixes #25388
Change summary
This change adds a necessary preprocessor check in src/credentials/examples/DeviceAttestationCredsExample.cpp to allow the code to build if
CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID
is set to a value outside of the range of test values i.e. [0x8000, 0x801F]. (seesrc/credentials/examples/ExampleDACs.cpp
) If we do not make this change, we end up with compilation failures like this when building an app (say the tv-casting-app) if the product id is outside the test range:Testing
Successfully able to build the tv-casting-app with a CHIP_DEVICE_CONFIG_DEVICE_PRODUCT_ID set to a value outside the range of test values, like 0x7FFF.