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

Add service proc macro #131

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

petekubiak
Copy link

@petekubiak petekubiak commented Oct 4, 2024

The purpose of this PR is to simplify the process for defining services and characteristics in TrouBLE, as laid out in this issue: #130.
Services are defined as a struct decorated with the #[gatt_service(uuid = ...)] attribute and fields within the struct can be decorated with #[characteristic(<args>)] attributes to define them as characteristics.
In a similar way, the gatt server is defined as a struct with the #[gatt_server] attribute, where each of the fields is a service.
The server is then instantiated with the ::new() function which is generated by the attribute macro.

This PR lays the foundation for expanding the functionality of the server, services and characteristics such that all access to characteristics (e.g. set, get, notify, indicate) can be done through methods on the characteristics, accessed through the server.

@plaes
Copy link
Contributor

plaes commented Oct 11, 2024

@petekubiak Could you split out the uuid parser/generator macro as separate PR?

This would immensely improve implementing services with 128-bit Uuids:

    let mut uart_service = table
        // #[gatt_service(uuid = "6E400001-B5A3-F393-E0A9-E50E24DCCA9E")] 
        .add_service(Service::new(Uuid::Uuid128([
                    0x9e, 0xca, 0xdc, 0x24,
                    0x0e, 0xe5, 0xa9, 0xe0,
                    0x93, 0xf3, 0xa3, 0xb5,
                    0x01, 0x00, 0x40, 0x6e,
            ])));

    // #[characteristic(uuid = "6E400002-B5A3-F393-E0A9-E50E24DCCA9E", write)]
    let _ = uart_service.add_characteristic(Uuid::Uuid128([
                    0x9e, 0xca, 0xdc, 0x24,
                    0x0e, 0xe5, 0xa9, 0xe0,
                    0x93, 0xf3, 0xa3, 0xb5,
                    0x02, 0x00, 0x40, 0x6e,
            ]),
            &[
                CharacteristicProp::Write,
            ],
            &mut rx,
        );

@alexmoon
Copy link

You can write .add_service(Service::new(Uuid::Uuid128(0x6E400001_B5A3_F393_E0A9_E50E24DCCA9Eu128.to_le_bytes()))). No macro needed.

@petekubiak
Copy link
Author

Following on from that my hope is that you wouldn't even need to call .add_service() or .add_characteristic() as the attribute macro takes care of that for you when you call <Server name>::new() (Server name being the name of the struct you have decorated with the #[gatt_server] attribute).

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Looks great, thanks the work on this! I've left a few inline comments.

I noticed a commend saying the pub server field would be non-public with additional changes. Do you wish to merge this now or make additional changes to it?

Cargo.toml.hidden Outdated Show resolved Hide resolved
examples/apps/src/ble_bas_peripheral.rs Outdated Show resolved Hide resolved
examples/apps/src/ble_derive_service.rs Outdated Show resolved Hide resolved
host-macro/Cargo.toml Outdated Show resolved Hide resolved
@petekubiak
Copy link
Author

petekubiak commented Oct 14, 2024

Looks great, thanks the work on this! I've left a few inline comments.

I noticed a commend saying the pub server field would be non-public with additional changes. Do you wish to merge this now or make additional changes to it?

Thanks! I think we'd like to merge this one first and then do further development in a new PR (avoids this one getting too large :) )

@petekubiak
Copy link
Author

Looks great, thanks the work on this! I've left a few inline comments.

I noticed a commend saying the pub server field would be non-public with additional changes. Do you wish to merge this now or make additional changes to it?

I've decided to set the visibilities back to what they were and instead implement getter functions for the fields I need. At least that way they are immutable.

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.

5 participants