CC26X2X7 Initial support of BLE Rendezvous #4865

srickardti · 2021-02-15T21:23:31Z

Migrate to the new Texas Instruments CC26X2X7 LaunchPad. This allows
concurrent Full Thread Device and BLE operation. This Enables BLE
rendezvous on the CC26X2X7.

Thanks to Alexander D'Abreu for work on the DMM integration and CHIPoBLE
profile development for the TI platform.

Co-Authored-by: Alexander D'Abreu alexander.dabreu@ti.com

Problem

The Texas Instruments CC26X2 platform does not enable BLE Rendezvous according to the CHIP specification.

Summary of Changes

Migrates the build to denote the new CC26X2X7 platform support
Enables concurrent Thread and BLE operation for the CC26X2 platform.
Adds CHIPoBLE profile for BLE Rendezvous for TI CC26X2 platform

Fixes #4864

Migrate to the new Texas Instruments CC26X2X7 LaunchPad. This allows concurrent Full Thread Device and BLE operation. This Enables BLE rendezvous on the CC26X2X7. Thanks to Alexander D'Abreu for work on the DMM integration and CHIPoBLE profile development for the TI platform. Co-Authored-by: Alexander D'Abreu <alexander.dabreu@ti.com>

src/platform/cc13x2_26x2/BLEManagerImpl.h

mspang

The recommended board changed?

examples/lock-app/cc13x2x7_26x2x7/README.md

mspang · 2021-02-16T16:37:38Z

src/platform/BUILD.gn

        "cc13x2_26x2/InetPlatformConfig.h",
        "cc13x2_26x2/Logging.cpp",
        "cc13x2_26x2/PlatformManagerImpl.cpp",
        "cc13x2_26x2/PlatformManagerImpl.h",
        "cc13x2_26x2/SystemPlatformConfig.h",
      ]

+      if (chip_enable_ble) {
+        configs -= [ "${build_root}/config/compiler:std_default" ]
+        configs += [ "//:sdk_posix_config" ]


Something wrong here, sdk_posix_config should be specified relative to some $foo_root.

This needs fixing. I think you just need to add "${sdk_target_name}_posix_config" to public_configs of "${chip_root}/third_party/ti_simplelink_sdk" rather than configs.

mspang · 2021-02-16T16:39:13Z

src/platform/cc13x2_26x2/BLEManagerImpl.cpp

+    err = BleLayer::Init(this, this, &SystemLayer);
+    if (err != CHIP_NO_ERROR)
+    {
+        while (1)


No better way to panic here?

mspang · 2021-02-16T16:40:24Z

src/platform/cc13x2_26x2/BLEManagerImpl.cpp

+    }
+    else
+    {
+        ret = CHIP_ERROR_INVALID_ARGUMENT;


Use buffer size too small error?

and one below

I'll update those, since the buffer size was statically allocated, we thought parameter would be a better fit.

mspang · 2021-02-16T16:41:49Z

src/platform/cc13x2_26x2/BLEManagerImpl.cpp

+
+    EnqueueEvtHdrMsg(BLEManagerIMPL_CHIPOBLE_CLOSE_CONN_EVT, (void *) pMsg);
+
+    return (CHIP_NO_ERROR);


nit: Remove extra parenthesis?

mis-match of our coding conventions from BLE, I'll remove them.

mspang · 2021-02-16T16:45:41Z

src/platform/cc13x2_26x2/BLEManagerImpl.cpp

+    memset(pBuf, 0x00, dataLen);
+    if (!pMsg || !pBuf)
+    {
+        while (1)


No better panic?

We could return a false. I will add this instead.

Would assert() or nlASSERT() be appropriate here?

I think we were unsure about what the best way forward is for these cases. For this one, returning a false is probably correct. There are others that the function does not have a return type, so we cannot propagate an error up. for that, would assert() be correct?

assert() should be preferred for any internal error or precondition failure. For public APIs, an error return may also be appropriate.

i.e., if the condition being false is not expected to happen unless there's a bug, we should assert.

Conversely, if an assert fires, it should be the case that there is a software bug or hardware failure

If an error condition happens that is not the result of a software bug, we must not assert.

So for these functions, returning a false on an allocation failure makes sense.

On some of the others, like failing to allocate the BLE task, I could add the return value path or assert?

I think it's reasonable to assert if we cannot allocate the task - it'd a bug if we hit this and not recoverable.

srickardti · 2021-02-16T18:06:21Z

@mspang Running the Thread stack and BLE stack together requires more Flash space than the CC2652R1 has available. The new CC2652R7 has 704K of flash to accommodate this requirement. Same platform with more Flash/RAM.

woody-apple

Approving pending @mspang

* Removed parens around return values. * Propagated an error if possible. * used assert() macro otherwise.

mspang

Request changes for the build file bit which should be fixed up.

mspang · 2021-02-17T00:31:35Z

src/platform/BUILD.gn

        "cc13x2_26x2/InetPlatformConfig.h",
        "cc13x2_26x2/Logging.cpp",
        "cc13x2_26x2/PlatformManagerImpl.cpp",
        "cc13x2_26x2/PlatformManagerImpl.h",
        "cc13x2_26x2/SystemPlatformConfig.h",
      ]

+      if (chip_enable_ble) {
+        configs -= [ "${build_root}/config/compiler:std_default" ]
+        configs += [ "//:sdk_posix_config" ]


This needs fixing. I think you just need to add "${sdk_target_name}_posix_config" to public_configs of "${chip_root}/third_party/ti_simplelink_sdk" rather than configs.

srickardti · 2021-02-17T16:58:57Z

Hey, had to deal with some stuff AFK. The state of Texas isn't in the best state right now. I was looking into it and found the header that needed the TI-POSIX API was removed somewhere along the line of developing this feature. But the build configuration wasn't updated. I've removed the offending build config.

The issue with adding the TI-POSIX config to the ${chip_root}/third_party/ti_simplelink_sdk target's public configs is that there isn't a way to propagate a negative requirement to the dependencies. The headers conflict with some of the GNU definitions pulled in by -std=gnu++14 and -std=gnu11.

mspang · 2021-02-17T17:08:32Z

Hey, had to deal with some stuff AFK. The state of Texas isn't in the best state right now. I was looking into it and found the header that needed the TI-POSIX API was removed somewhere along the line of developing this feature. But the build configuration wasn't updated. I've removed the offending build config.

The issue with adding the TI-POSIX config to the ${chip_root}/third_party/ti_simplelink_sdk target's public configs is that there isn't a way to propagate a negative requirement to the dependencies. The headers conflict with some of the GNU definitions pulled in by -std=gnu++14 and -std=gnu11.

Ah, right, got it. The main thing that needs fixing is just using // to refer to it instead of simplelink_sdk_root, chip_root, or similar. I don't want us to depend on where the code is in the checkout, in general, so we have the foo_root variables to avoid it.

You can put a config in third_party/simplelink and reference it via simplelink_root. Or, for things that the application might replace, we have the option of using declare_args() to let the application provide the target label, and importing that declaration from a known path. That's the only way we should be referring to an application provided library or config target from common code such as src/platform.

mspang · 2021-02-17T17:10:28Z

Hey, had to deal with some stuff AFK. The state of Texas isn't in the best state right now. I was looking into it and found the header that needed the TI-POSIX API was removed somewhere along the line of developing this feature. But the build configuration wasn't updated. I've removed the offending build config.
The issue with adding the TI-POSIX config to the ${chip_root}/third_party/ti_simplelink_sdk target's public configs is that there isn't a way to propagate a negative requirement to the dependencies. The headers conflict with some of the GNU definitions pulled in by -std=gnu++14 and -std=gnu11.

Ah, right, got it. The main thing that needs fixing is just using // to refer to it instead of simplelink_sdk_root, chip_root, or similar. I don't want us to depend on where the code is in the checkout, in general, so we have the foo_root variables to avoid it.

You can put a config in third_party/simplelink and reference it via simplelink_root. Oe, for things that the application might replace, we have the option of using declare_arg() to point a group target at it. That's the only way we should be referring to an application provided library or config target from common code such as src/platform.

BTW, can we just replace gnu++14 with c++14 for the whole TI build, not on a target by target basis? If so, that's a good solution.

jelderton · 2021-02-17T19:12:06Z

src/platform/cc13x2_26x2/BLEManagerImpl.cpp

+    pBuf = (uint8_t *) ICall_malloc(dataLen);
+    if (NULL == pBuf)
+    {
+        ICall_free((void *) pBuf);


is this suppose to be ICall_free((void *) pMsg);

good catch, yes it is.

Thanks for the quick response. Waiting for all of the checks to complete, and I'll merge this in.

This broke after e76ed54 ("CC26X2X7 Initial support of BLE Rendezvous (project-chip#4865)") Tested via gn gen out/unified --args="target_os=\"all\" enable_ti_simplelink_builds=true ti_simplelink_sdk_root=\"$HOME/ti/simplelink_cc13x2_26x2_sdk_4_40_05_02_eng\" ti_sysconfig_root=\"$HOME/ti/sysconfig_1.7.0\" enable_linux_bridge_app_build=false"

This broke after e76ed54 ("CC26X2X7 Initial support of BLE Rendezvous (project-chip#4865)") ERROR at //config/cc13x2_26x2/toolchain/BUILD.gn:23:5: Unable to load "/home/spang/connectedhomeip2/examples/lock-app/cc13x2_26x2/args.gni". import("${chip_root}/examples/lock-app/cc13x2_26x2/args.gni") ^----------------------------------------------------------- See //BUILD.gn:245:16: which caused the file to be included. deps = [ "${chip_root}/examples/lock-app/cc13x2_26x2(${chip_root}/config/cc13x2_26x2/toolchain:cc13x2_26x2_lock_app)" ] ^-------------------------------------------------------------------------------- Fix the build file to use the new paths. Tested via gn gen out/unified --args="target_os=\"all\" enable_ti_simplelink_builds=true ti_simplelink_sdk_root=\"$HOME/ti/simplelink_cc13x2_26x2_sdk_4_40_05_02_eng\" ti_sysconfig_root=\"$HOME/ti/sysconfig_1.7.0\" enable_linux_bridge_app_build=false"

This broke after e76ed54 ("CC26X2X7 Initial support of BLE Rendezvous (#4865)") ERROR at //config/cc13x2_26x2/toolchain/BUILD.gn:23:5: Unable to load "/home/spang/connectedhomeip2/examples/lock-app/cc13x2_26x2/args.gni". import("${chip_root}/examples/lock-app/cc13x2_26x2/args.gni") ^----------------------------------------------------------- See //BUILD.gn:245:16: which caused the file to be included. deps = [ "${chip_root}/examples/lock-app/cc13x2_26x2(${chip_root}/config/cc13x2_26x2/toolchain:cc13x2_26x2_lock_app)" ] ^-------------------------------------------------------------------------------- Fix the build file to use the new paths. Tested via gn gen out/unified --args="target_os=\"all\" enable_ti_simplelink_builds=true ti_simplelink_sdk_root=\"$HOME/ti/simplelink_cc13x2_26x2_sdk_4_40_05_02_eng\" ti_sysconfig_root=\"$HOME/ti/sysconfig_1.7.0\" enable_linux_bridge_app_build=false"

This broke after e76ed54 ("CC26X2X7 Initial support of BLE Rendezvous (project-chip#4865)") ERROR at //config/cc13x2_26x2/toolchain/BUILD.gn:23:5: Unable to load "/home/spang/connectedhomeip2/examples/lock-app/cc13x2_26x2/args.gni". import("${chip_root}/examples/lock-app/cc13x2_26x2/args.gni") ^----------------------------------------------------------- See //BUILD.gn:245:16: which caused the file to be included. deps = [ "${chip_root}/examples/lock-app/cc13x2_26x2(${chip_root}/config/cc13x2_26x2/toolchain:cc13x2_26x2_lock_app)" ] ^-------------------------------------------------------------------------------- Fix the build file to use the new paths. Tested via gn gen out/unified --args="target_os=\"all\" enable_ti_simplelink_builds=true ti_simplelink_sdk_root=\"$HOME/ti/simplelink_cc13x2_26x2_sdk_4_40_05_02_eng\" ti_sysconfig_root=\"$HOME/ti/sysconfig_1.7.0\" enable_linux_bridge_app_build=false" GitOrigin-RevId: 2891d6b

boring-cyborg bot added the examples label Feb 15, 2021

restyled-commits added 3 commits February 15, 2021 21:24

Restyled by clang-format

ded88b0

Restyled by gn

fa8b780

Restyled by prettier-markdown

d132f29

restyled-io bot mentioned this pull request Feb 15, 2021

Restyle CC26X2X7 Initial support of BLE Rendezvous #4866

Closed

fixup doxygen issues

bbfce75

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, saurabhst and woody-apple February 15, 2021 22:47

saurabhst approved these changes Feb 16, 2021

View reviewed changes

andy31415 approved these changes Feb 16, 2021

View reviewed changes

src/platform/cc13x2_26x2/BLEManagerImpl.h Outdated Show resolved Hide resolved

remove commented out code

191e245

restyled-io bot mentioned this pull request Feb 16, 2021

Restyle CC26X2X7 Initial support of BLE Rendezvous #4882

Closed

mspang reviewed Feb 16, 2021

View reviewed changes

woody-apple approved these changes Feb 16, 2021

View reviewed changes

srickardti and others added 3 commits February 16, 2021 14:46

Update return cases

2941104

* Removed parens around return values. * Propagated an error if possible. * used assert() macro otherwise.

Restyled by whitespace

e9d9cdb

Restyled by clang-format

de78c3f

mspang requested changes Feb 17, 2021

View reviewed changes

remove unneded TI-POSIX conifg

85c27df

srickardti requested a review from mspang February 17, 2021 16:52

mspang approved these changes Feb 17, 2021

View reviewed changes

msandstedt approved these changes Feb 17, 2021

View reviewed changes

jelderton reviewed Feb 17, 2021

View reviewed changes

free correct buffer in error case

4a9bbd9

jelderton approved these changes Feb 17, 2021

View reviewed changes

Merge remote-tracking branch 'gh/master'

e157f30

mspang merged commit e76ed54 into project-chip:master Feb 17, 2021

mspang mentioned this pull request Feb 23, 2021

Fix TI simplelink in unified build #4983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CC26X2X7 Initial support of BLE Rendezvous #4865

CC26X2X7 Initial support of BLE Rendezvous #4865

srickardti commented Feb 15, 2021

mspang left a comment

mspang Feb 16, 2021

mspang Feb 17, 2021

mspang Feb 16, 2021

mspang Feb 16, 2021

srickardti Feb 16, 2021

mspang Feb 16, 2021

srickardti Feb 16, 2021

mspang Feb 16, 2021

srickardti Feb 16, 2021

mspang Feb 16, 2021

srickardti Feb 16, 2021

mspang Feb 16, 2021

srickardti Feb 16, 2021

mspang Feb 16, 2021

srickardti commented Feb 16, 2021

woody-apple left a comment

mspang left a comment

mspang Feb 17, 2021

srickardti commented Feb 17, 2021

mspang commented Feb 17, 2021 •

edited

Loading

mspang commented Feb 17, 2021

jelderton Feb 17, 2021

srickardti Feb 17, 2021

jelderton Feb 17, 2021


		EnqueueEvtHdrMsg(BLEManagerIMPL_CHIPOBLE_CLOSE_CONN_EVT, (void *) pMsg);

		return (CHIP_NO_ERROR);

CC26X2X7 Initial support of BLE Rendezvous #4865

CC26X2X7 Initial support of BLE Rendezvous #4865

Conversation

srickardti commented Feb 15, 2021

Problem

Summary of Changes

mspang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srickardti commented Feb 16, 2021

woody-apple left a comment

Choose a reason for hiding this comment

mspang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srickardti commented Feb 17, 2021

mspang commented Feb 17, 2021 • edited Loading

mspang commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspang commented Feb 17, 2021 •

edited

Loading