nRF Connect SDK contribution on github - CTS client LTI support

[Tamisoft]

Hi,

we have made a pull request on github to introduce the LTI support in CTS client, since one of our new devices have a display and needed a user friendly way of supporting accurate time. Noticed that CTS client implementation doesn't have the optional LTI support.

The GH PR is bluetooth: Add Local Time Information support to CTS client by tamisoft · Pull Request #24146 · nrfconnect/sdk-nrf · GitHub

The only problem we saw is that during the internal build testing it was trying to fetch some documentation that hasn't been published yet, so it got a permission denied. This is unrelated to the changes introduced, all other tests passed. 

Question is, what is the way to get this kind of code contributed to the SDK? Originally 2 guys were active with the PR then big silence, we updated the latest masters a couple of times, but nothing. 

Is there something else needed to get something like this accepted, we are willing to comply with whatever the rules might be. We have followed the suggested zephyr contribution steps.

Br 

Levente

Offcode Oy

[Elfving]

Hi,

We do appreciate when people like you contribute to the SDK, but unfortunately, we aren't that good at receiving this help at the moment. 

We need someone to review this PR, and we haven't been able to prioritize that yet. The way you are doing it is the right way, though. 

Contributing directly to zephyr might be easier, as that is more focused on help from externals. Though I am not sure if what you are addressing here has a perfect equivalent in the Zephyr repo.

Regards,

Elfving

[Tamisoft]

I really appreciate the prompt feedback here, and I understand it can be complicated. I was considering contributing it to zephyr, but they have a different implementation completely (though that is the same in the way that also only implements the mandatory characteristics).

Let's see how it'll go, we are trying to be productive and helpful, so basically anything goes. Our current workaround is that we do have in the CI/CD pipeline a pre-build step to apply a set of patches to the sdk before the build, but it gets cumbersome, and I think these kinds of changes have value for others as well.

Regards

Levi

[Elfving]

Tamisoft said:

Let's see how it'll go, we are trying to be productive and helpful, so basically anything goes.

It is really appreciated. And that is the case no matter where you help out. But maybe it is particularly helpful in the nrf repo in a way, because it's up to us to address the issues there eventually. We might get back to this PR, either when we are forced to prioritize this, or simply have the time to. So I wouldn't say anything is wasted even if we do not get to it right away. 

Feel free to address this in zephyr too if you want to.

Tamisoft said:

Our current workaround is that we do have in the CI/CD pipeline a pre-build step to apply a set of patches to the sdk before the build, but it gets cumbersome, and I think these kinds of changes have value for others as well.

Yeah, considering the comment by 'reavertm', it definitely seems like it

Regards,

Elfving

[Tamisoft]

Elfving said:

Yeah, considering the comment by 'reavertm', it definitely seems like it

I guess one of the driving force behind this particular feature is that iOS/MacOS by default implements the service in their bt stack, so this is an easy way to sync time to connected devices. The mandatory char part doesn't cut it if you need the human compatible time (unless you live in UTC :D )

Should we close this ticket here, or wait until something happens in the GH PR and then do it?

Regards,

Levi

[Elfving]

Ah I see. Sounds like a bonus that is easy to overlook unless you are already aware of the default implementation in iOS  :/ Hopefully the PR won't be completly forgotten. Otherwise, if you are able to fit it into Zephyr that would still be great.

We can just keep this open in case there is any development. Since there are two of you who had the same idea for a fix, there might be more in the future.

Regards,

Elfving