add locomo2 board info - Githubissues

justinleeyang commented 5 years ago

add locomo2 board info

ncasaril commented 5 years ago

Hi Justin, You're adding a bsp and making changes to the gpio layer in the same pull request. Please split this pull request into two separate requests. And as such, please have a clean commit for the gpio changes that doesn't involve changes to the bsp at the same time. Makes it hard to check otherwise. Thanks, Niklas

justinleeyang commented 5 years ago

hi, ncasaril.

Sorry, Got it, thanks!

ncasaril commented 5 years ago

Hi Justin,

Thank you for your work on this.

May I suggest you split the function dw1000_support_pa_lna into two functions, one for enabling external PA and one for enabling external LNA, as not all boards sporting a LNA has a PA and vice versa. I'd prefer the functions to be of the format:

dw1000_phy_enable_ext_pa(struct _dw1000_dev_instance* inst, bool enable);
dw1000_phy_enable_ext_lna(struct _dw1000_dev_instance* inst, bool enable);

Where the enable flag allows the enabling / disabling of the feature. And, these functions shouldn't live in dw1000_gpio.c but in dw1000_phy.c, as suggested in the function names above.

Ideally, I'd prefer if you submit a separate pull request just for the gpio changes and then the pull request for adding support for the locomo2 board. But, that's not essential as long as the gpio changes are clean :)

Thanks, Niklas

justinleeyang commented 5 years ago

hi, ncasaril

I have separated the pa and lna function, more function name as follow: dw1000_gpio4_config_ext_pa dw1000_gpio5_config_ext_txe dw1000_gpio6_config_ext_rxe

justinleeyang commented 5 years ago

hi, ncasaril

Because I don't know your more code architecture and rules, So I can only add code and functionality as I understand it. you can provide more details for me.

pkettle commented 5 years ago

Hi Justin, As Niklas points out you are creating a board depencency from within the core library by referencing individual pins. The pin assignments may align with your hardware but likely not with others. The Mynewt approach and move all such BPS dependencies to the your/bsp/syscfg.yml

For example you should choose a platform agnostics naming convention

dw1000_phy_enable_ext_pa(struct _dw1000_dev_instance* inst, bool enable);
dw1000_phy_enable_ext_lna(struct _dw1000_dev_instance* inst, bool enable);

Then from within your/bsp/syscfg.yml add:

DW1000_DEVICE_0_PA:
    description: 'External PA pin'
    value: 5
DW1000_DEVICE_0_LNA:
    description: 'External LNA pin'
    value: -1

And from within hw/dw1000/dw1000_phy.c add the implementation:

dw1000_phy_enable_ext_pa(struct _dw1000_dev_instance* inst, bool enable){
    // Within the implementation you reference the PA pin via.
    uint8_t pa_pin = MYNEWT_VAL(DW1000_DEVICE_0_PA);
    ...
}

dw1000_phy_enable_ext_lna(struct _dw1000_dev_instance* inst, bool enable){
    // Within the implementation you reference the LNA pin via.
    uint8_t lna_pin = MYNEWT_VAL(DW1000_DEVICE_0_LNA);
    ...
}

justinleeyang commented 5 years ago

hi, pkettle

the gpio is dw1000's gpio4/5/6, don't my BPS dependencies.

pkettle commented 5 years ago

Hi Justin, It is likely that gpio4/5/6 are used by other people for another purpose (GeneralPurposeInputOutput). We should not make assumptions about this purpose and keep things generic. The bsp allow you to map these pins to your board.

justinleeyang commented 5 years ago

ok

pkettle commented 5 years ago

Hi Justin, I have merged your request. We may revisit the API at some point in the future as we see move PA/LNA capable boards.

Decawave / mynewt-dw1000-core

add locomo2 board info #5