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.

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

Jump to bottom

intended form of zero in bitfields? #5

Closed
karlp opened this issue Aug 28, 2021 · 5 comments · May be fixed by #6
Closed

intended form of zero in bitfields? #5

karlp opened this issue Aug 28, 2021 · 5 comments · May be fixed by #6

Comments

@karlp
Copy link

karlp commented Aug 28, 2021

Hi,

I'm porting some code that sets the characteristic event flags. This is the following in C,

/* Gatt Event Mask
 * Type of event generated by GATT server
 * See aci_gatt_add_char.
 */
#define GATT_DONT_NOTIFY_EVENTS                       0x00 
#define GATT_NOTIFY_ATTRIBUTE_WRITE                   0x01
#define GATT_NOTIFY_WRITE_REQ_AND_WAIT_FOR_APPL_RESP  0x02
#define GATT_NOTIFY_READ_REQ_AND_WAIT_FOR_APPL_RESP   0x04

In the rust package, it's a bitfield: https://github.com/eupn/stm32wb55/blob/master/src/command/gatt.rs#L1565

    pub struct CharacteristicEvent: u8 {
        const ATTRIBUTE_WRITE = 0x01;
        const CONFIRM_WRITE = 0x02;
        const CONFIRM_READ = 0x04;
    }

What's the "intended" way of writing this, when you want to use the "zero" form? You can use a literal 0, but that's not as clear as the "CharacteristicEvent::NO_NOTIFY" or similar?
@karlp
Copy link
Author

karlp commented Aug 28, 2021

I've usd CharacteristicEvent::empty() but ... is that really the best way?

@eupn
Copy link
Owner

eupn commented Aug 28, 2021

@karlp does this looks ok? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=74f2593ee25960645b5d838d74db1640

@karlp
Copy link
Author

karlp commented Aug 29, 2021

sure! that's perfectly fine. I didn't read up on bitfields enough toknow that the zeros could be put in them too, I'm pretty new to rust :)

@eupn eupn closed this as completed Aug 30, 2021
@karlp
Copy link
Author

karlp commented Aug 31, 2021

Were you going to add this to the module? or just show how I could do it in general? Should also apply to CharacteristicPermission as well?

Looking at AN5270 for ACI_GATT_ADD_CHAR, it should get 0s defined for CharacteristicProperty, CharacteristicPermission, CharacteristicEvent,

For ACI_GATT_ADD_CHAR_DESC, there's 0, and some higher numbers missing from DescriptorPermission, AccessPermission

For ACI_GATT_UPDATE_CHAR_VALUE_EXT, the 0 should be added to UpdateType

I can make a PR for this is you'd prefer?

@eupn
Copy link
Owner

eupn commented Aug 31, 2021

@karlp

I can make a PR for this is you'd prefer?

Sure, go ahead! That would be great!

karlp added a commit to etactica/stm32wb55 that referenced this issue Aug 31, 2021
@karlp
gatt: add missing bit definitions
7791ccd 
Based on AN5270rev10

For ACI_GATT_ADD_CHAR, added the "NONE" properties, and the "NO_NOTIFY"
gatt event mask.

For ACI_GATT_ADD_CHAR_DESC, reused the same security bitflags, and
extended the access bitflags.

Signed-off-by: Karl Palsson <[email protected]>
Fixes: eupn#5
@karlp karlp mentioned this issue Aug 31, 2021
Open
karlp added a commit to etactica/stm32wb55 that referenced this issue Aug 31, 2021
@karlp
gatt: add missing bit definitions
757bfeb 
Based on AN5270rev10

For ACI_GATT_ADD_CHAR, added the "NONE" properties, and the "NO_NOTIFY"
gatt event mask.

For ACI_GATT_ADD_CHAR_DESC, reused the same security bitflags, and
extended the access bitflags.

Signed-off-by: Karl Palsson <[email protected]>
Fixes: eupn#5
karlp added a commit to etactica/stm32wb55 that referenced this issue Sep 1, 2021
@karlp
gatt: add missing bit definitions
6704cf9 
Based on AN5270rev10

For ACI_GATT_ADD_CHAR, added the "NONE" properties, and the "NO_NOTIFY"
gatt event mask.

For ACI_GATT_ADD_CHAR_DESC, reused the same security bitflags, and
extended the access bitflags.

Signed-off-by: Karl Palsson <[email protected]>
Fixes: eupn#5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gatt: add missing bit definitions etactica/stm32wb55
2 participants
@karlp @eupn