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.

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

Disable QPG6100 in CI #6235

Closed
wants to merge 1 commit into from
Closed

Disable QPG6100 in CI #6235

wants to merge 1 commit into from

Conversation

yufengwangca
Copy link
Contributor

@yufengwangca yufengwangca commented Apr 22, 2021

Problem

We are currently close to the Flash threshold on QPG6100 (total 524K flash budget), but we need to enable a series of must-to-have features in CHIP sdk to comply with spec, such as group messaging and Interaction Model ...

We have notified SOC vendors that at least 1M flash and 128K RAM (preferably 256K) are required to add their platforms.

Summary of Changes

Disable QPG6100 in CI to unblock feature development

@yufengwangca yufengwangca marked this pull request as draft April 22, 2021 16:50
@yufengwangca yufengwangca marked this pull request as ready for review April 22, 2021 17:04
@andy31415 andy31415 requested a review from tima-q April 22, 2021 19:46
@andy31415
Copy link
Contributor

Added Timothy for feedback - are there alternatives? Could we have a part attached to more SPI flash that we could still use to be able to continue run compilation tests?

@yunhanw-google
Copy link
Contributor

@bzbarsky-apple @msandstedt ?

@bzbarsky-apple
Copy link
Contributor

I'd like to hear what @tima-q has to say.

Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we turn off platforms in CI, have we done a recent analysis about where space is being used? There's been a lot of growth recently, and it seems like we could spend some effort there, rather than simply declare bankruptcy?

@andy31415
Copy link
Contributor

Before we turn off platforms in CI, have we done a recent analysis about where space is being used? There's been a lot of growth recently, and it seems like we could spend some effort there, rather than simply declare bankruptcy?

@woody-apple I added you to an e-mail thread. The status we have is that we are already at the limit and the spec mandated features include code changes that are expected to be non-trivial. An incomplete list includes:

  • ACL table, including possible in-memory caching (unless it is always validated by going to fetch from NV store)
  • CASE session context for each peer, where a peer is any Node trying to concurrently have a running session with the device
    Group key storage
  • OpCert storage (right now, there are zero opcerts accounted for at all)
  • Unencrypted session contexts for CASE/PASE establishment
  • IPv6 prefix/route maintenance for Thread off-mesh prefixes

For this reason, we think we are likely to be unable to make small increments while still keeping within memory limits. I expect we will still want to make it fit, but likely as a cleanup phase rather than enforcing it for all new PRs.

@andy31415
Copy link
Contributor

That being said, maybe we should keep the changes requested flag for a while to give @tima-q a chance to chime in and prvide alternatives. I just want to make sure that we do not end up blocking all new commits.

@andy31415
Copy link
Contributor

image

Here is how the QPG map looked like yesterday (not 100% complete, but a general idea of size distribution)

@andy31415
Copy link
Contributor

I believe there may be 'optimized build' ideas like dropping OT CLI (22K savings), removing TCP support and keep only UDP (10K savings) and maybe more (e.g. rodata not shown here ... probably removing debug strings and replacing with placeholders will save an unknown amount of space).

However some of these seem to have the potential to make debugging harder while things are moving fast, so I would prefer to apply them a bit later.

@tima-q
Copy link
Contributor

tima-q commented Apr 23, 2021

We've put up #6261 to alleviate immediate PR's currently blocked. 26kB flash/2kB static RAM is freed up there.
@kpschoedel Thank you for the pointer into lwip TCP useage. There might be other optimizations for Thread/BLE only devices that we are not aware of yet.

@yufengwangca Could you point me to the communication you refer to for internal reference ?
I myself was only aware of the earlier communicated assumption of 512kB/64kB (previously 256kB/64kB)

@andreilitvin If you now disable QPG6100, it is with the working assumption the code scales back down to 512kB again right ? What would be the gatekeeper for that? Moving to more then 512kB even affects 1MB parts for in-place OTA upgrades.

@andy31415
Copy link
Contributor

We've put up #6261 to alleviate immediate PR's currently blocked. 26kB flash/2kB static RAM is freed up there.
@kpschoedel Thank you for the pointer into lwip TCP useage. There might be other optimizations for Thread/BLE only devices that we are not aware of yet.

@yufengwangca Could you point me to the communication you refer to for internal reference ?
I myself was only aware of the earlier communicated assumption of 512kB/64kB (previously 256kB/64kB)

@andreilitvin If you now disable QPG6100, it is with the working assumption the code scales back down to 512kB again right ? What would be the gatekeeper for that? Moving to more then 512kB even affects 1MB parts for in-place OTA upgrades.

Regarding sizes: I would absolutely like to have the smallest possible usage size. 512 seems maybe possible, but will require significant work and likely some very stringent limits (like no TCP, heavy tuning of debug strings, buffer sizes etc). At this point I am not sure we can guarantee fitting in 512K given the features that we have to implement. It is highly desirable though to ensure that chip device builds are more reasonable.

For communication, Yufeng is probably referring to additional silicon vendors looking to add their chip into the SDK for CI and for those I specifically asked for 1MB flash and 128K+ RAM so that we do not block PRs on additional CI items. There is no official communication or guidance beyond what you stated as 512K/64K.

@yufengwangca
Copy link
Contributor Author

#6261 freed up 26kB Flash and 2kB RAM, and this allow us continue our feature development on QPG6100. Close this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants