NordicSemiconductor / pc-ble-driver-py

Python bindings for the ble-driver library
Other
126 stars 115 forks source link

BLEGattsAttrMD: read_perm and write_perm #184

Closed dangu closed 3 years ago

dangu commented 3 years ago

When trying to read a characteristic I was getting READ_NOT_PERMITTED. I would like to discuss how to add the functionality of BLE_GAP_CONN_SEC_MODE_SET_OPEN() and friends to pc_ble_driver_py, to control the read_perm and write_perm of BLEGattsAttrMD.

I have made some tests with adding this line to BLEGattsAttrMD:

attr_md.read_perm = driver.ble_gap_conn_sec_mode_t()

After that, it is possible to assign the sm and lv properties:

 attr_md.read_perm.sm=1
 attr_md.read_perm.lv=1

I also started implementing a class BLEGapConnSecMode for the ble_gap_conn_sec_mode_t but now I need help. How do I proceed?

Edit: Ok, I think I start to understand the structure of this. What do you think about my suggested set_no_access() and set_open() methods in BLEGapConnSecMode? Is this the way to go? This allows for this kind of code:

    read_perm = BLEGapConnSecMode()
    read_perm.set_open()
    write_perm = BLEGapConnSecMode()
    write_perm.set_open()

    attr_md = BLEGattsAttrMD(read_perm=read_perm, write_perm=write_perm)
    attr = BLEGattsAttr(uuid=char_128_uuid, attr_md=attr_md,
                        max_len=max_len, value=value)
    self.adapter.driver.ble_gatts_characteristic_add(
                        serv_128_handle.handle,
                        char_md, attr, char_128_handles)
CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

bihanssen commented 3 years ago

@dangu thanks for the PR. I think your proposed solution looks sound.

dangu commented 3 years ago

Oh, I see now that "master" has been merged into "feature/conn_sec_mode". Is this really the way to do this? This means I need to take care to not break anything if I push a fix for the above review comments, right?

Edit: Tried pushing a fix for the above comments.

bihanssen commented 3 years ago

@dangu, merging master into this branch is no problem. It’s anyway required before eventually merging into master.

bihanssen commented 3 years ago

@dangu I added some changes to the PR. Will merge this to master now.