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

samples: alexa_gadget: Use custom_command for proto compilation #3392

Merged
merged 1 commit into from Dec 7, 2020

Conversation

nordic-auko
Copy link
Contributor

Change CMake protobuf compile invocation from custom_target
to custom_command. This improves dependency handling of
.proto files and solves compile issue seen in SES.

Signed-off-by: Audun Korneliussen audun.korneliussen@nordicsemi.no

Copy link
Contributor

@joerchan joerchan left a comment

Choose a reason for hiding this comment

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

Minor improvement suggested.

@nordic-auko Is the inclusion and compilation something that should be moved out of the sample directory perhaps, so that it would be easier for the application to include this.
Now I guess the application will have to copy this cmakelists.txt?

@nordic-auko
Copy link
Contributor Author

@nordic-auko Is the inclusion and compilation something that should be moved out of the sample directory perhaps, so that it would be easier for the application to include this.
Now I guess the application will have to copy this cmakelists.txt?

Yes, currently an application that wants to add support for the Alexa Gadgets functionality needs to copy the sample/src/gadgets_profile folder, which is not ideal.

The profile was put under the sample because there is no bluetooth/profiles location similar to bluetooth/services.

I hesitated to combine the Gadgets service.c and profile.c into one service.c file because this Profile dictates a number of things that doesn't make sense to put under subsys/bluetooth/services. For example, advertisement format, pairing mode, and MTU exchange behavior needs to adhere to the Gadgets Profile in order for the interaction with the peer to work properly.

It is possible to combine the service.c and profile.c file into one service under bluetooth/services, but move the MTU, advertisement format etc. out to sample/main.c. However this means it becomes more complex to include the functionaliy in another sample/application as you need to copy relevant code from the sample main.c in addition to adding the relevant Kconfigs.

As a side note regarding protobuf compilation, it could be worthwhile to have a cmake command for this, same as generate_inc_file_for_target() can be used to generate arrays from binary files.

@joerchan
Copy link
Contributor

@nordic-auko We have profiles in the services folder. So it could be moved there, and as separate source files.
For example services has HIDS and HOGP (HID service and HID over GATT profile).

If it is easier can also make a subfolder under services for alex_profile and have the cmakelists.txt there.

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

with changed code, please switch to use: WORKING_DIRECTORY. as the previous reason for using cmake -E chdir no longer applies with new code.

@nordic-auko
Copy link
Contributor Author

@nordic-auko We have profiles in the services folder. So it could be moved there, and as separate source files.
For example services has HIDS and HOGP (HID service and HID over GATT profile).

If it is easier can also make a subfolder under services for alex_profile and have the cmakelists.txt there.

Thanks, I did not realize there was a hogp.c there.
I can ponder the details a bit and set up a separate PR to move the gadgets profile out of the sample.
For example, there is a "custom event" protobuf type that might make sense to keep in the sample such that it can be easily overridden by an application.

@nordic-auko
Copy link
Contributor Author

with changed code, please switch to use: WORKING_DIRECTORY. as the previous reason for using cmake -E chdir no longer applies with new code.

Updated as suggested

@nordic-auko nordic-auko reopened this Nov 25, 2020
@nordic-auko
Copy link
Contributor Author

Closed by accident..

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Looks good.

Two small suggestions, but nothing blocking.

samples/bluetooth/alexa_gadget/src/gadgets/CMakeLists.txt Outdated Show resolved Hide resolved
samples/bluetooth/alexa_gadget/src/gadgets/CMakeLists.txt Outdated Show resolved Hide resolved
Change CMake protobuf compile invocation from custom_target
to custom_command. This improves dependency handling of
.proto files and solves compile issue seen in SES.

Signed-off-by: Audun Korneliussen <audun.korneliussen@nordicsemi.no>
@joerchan joerchan merged commit 887dbee into nrfconnect:master Dec 7, 2020
@nordic-auko nordic-auko deleted the gadgets_cmake_proto_target branch October 7, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants