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

Figure out how QR code parsing can be used safely with the MemoryInit requirement #3398

Closed
bzbarsky-apple opened this issue Oct 22, 2020 · 6 comments · Fixed by #3421
Closed

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

QR code parsing was changes to use Platform::Memory bits, but that requires a MemoryInit call. Unfortunately, scanning and parsing of a QR code can happen independently of overall CHIP stack bringup. This was mentioned as a possible issue in #3397 (comment) and we ran into this exact issue in the iOS CHIPTool as well.

Proposed Solution

Allow multiple calls to MemoryInit, with the ones after the first call no-ops except for incrementing a counter. Have MemoryShutdown decrement the counter; in case it ever becomes nontrivial, the nontrivial actions will be taken only when the counter goes to 0. Once that's done, the QR code parsing bits can init and shutdown around whatever work they need to do.

Alternately, we could go back to not using Platform::Memory for QR code parsing, since that's not going to be running on constrained devices....

@andy31415 thoughts?
@sagar-apple

@issue-label-bot
Copy link

Issue-Label Bot is automatically applying the label feature_request to this issue, with a confidence of 0.57. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.

@bzbarsky-apple
Copy link
Contributor Author

@kpschoedel also. #3260 talks about "embedded device-side code", which src/setup_payload/QRCodeSetupPayloadParser.cpp doesn't seem to be.

@kpschoedel
Copy link
Contributor

kpschoedel commented Oct 22, 2020

This is one reason I don't really like CHIPMem.h — it makes it harder to combine CHIP-library code with non-CHIP code.

That being said, my current intent for #3178 (Use mbedtls_platform_set_calloc_free() for non-malloc() configurations), after sleeping on it, is

  • rename the allocator-implementation MemoryInit() and MemoryShutdown() in CHIPMem-Malloc.cpp and CHIPMem-SimpleAlloc.cpp
  • wrap these in an allocator-implementation–independent MemoryInit() and MemoryShutdown() that also performs configuration-specific operators like calling mbedtls_platform_set_calloc_free().

In this case, the generic MemoryInit() and MemoryShutdown() can easily ensure that they call the implementation functions only once.

@bzbarsky-apple
Copy link
Contributor Author

Another possible API is to allow arbitrarily many calls to MemoryInit but have MemoryShutdown shut things down immediately. This is what CHIPMem-SimpleAlloc seems to implement in practice....

@bzbarsky-apple
Copy link
Contributor Author

@kpschoedel That would work for the use cases here, I think. @sagar-apple pointed out that at least for the "show QR code on screen" use case the device does need to use src/setup_payload/QRCodeSetupPayloadGenerator.cpp, which presumably ends up poking at memory bits via TLV.

@kpschoedel
Copy link
Contributor

Another possible API is to allow arbitrarily many calls to MemoryInit but have MemoryShutdown shut things down immediately. This is what CHIPMem-SimpleAlloc seems to implement in practice....

Assuming it gets wrapped anyway, the allocator implementations can assume no nested calls, and the specification of MemoryShutdown can be independent of the allocator implementation.

kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Oct 23, 2020
#### Problem

Some code wants to use `CHIPMem.h` allocation in contexts where
higher-level code might or might not already have initialized it.
Allowing multiple calls (proposed in issue project-chip#3398) solves this.

#### Summary of Changes

- Rename the allocator-specific `MemoryInit()` and `MemoryShutdown()`.
- Call those renamed functions from an allocator-independent
  `MemoryInit()` and `MemoryShutdown()` that allow mulitple calls
  and invoke the underlying allocator only once.

Fixes project-chip#3398 — Figure out how QR code parsing can be used safely with the MemoryInit requirement
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Oct 23, 2020
#### Problem

Some code wants to use `CHIPMem.h` allocation in contexts where
higher-level code might or might not already have initialized it.
Allowing multiple calls (proposed in issue project-chip#3398) solves this.

#### Summary of Changes

- Rename the allocator-specific `MemoryInit()` and `MemoryShutdown()`.
- Call those renamed functions from an allocator-independent
  `MemoryInit()` and `MemoryShutdown()` that allow mulitple calls
  and invoke the underlying allocator only once.

Fixes project-chip#3398 — Figure out how QR code parsing can be used safely with the MemoryInit requirement
kpschoedel added a commit to kpschoedel/connectedhomeip that referenced this issue Oct 23, 2020
#### Problem

Some code wants to use `CHIPMem.h` allocation in contexts where
higher-level code might or might not already have initialized it.
Allowing multiple calls (proposed in issue project-chip#3398) solves this.

#### Summary of Changes

- Rename the allocator-specific `MemoryInit()` and `MemoryShutdown()`.
- Call those renamed functions from an allocator-independent
  `MemoryInit()` and `MemoryShutdown()` that allow mulitple calls
  and invoke the underlying allocator only once.

Fixes project-chip#3398 — Figure out how QR code parsing can be used safely with the MemoryInit requirement
BroderickCarlin pushed a commit that referenced this issue Oct 26, 2020
#### Problem

Some code wants to use `CHIPMem.h` allocation in contexts where
higher-level code might or might not already have initialized it.
Allowing multiple calls (proposed in issue #3398) solves this.

#### Summary of Changes

- Rename the allocator-specific `MemoryInit()` and `MemoryShutdown()`.
- Call those renamed functions from an allocator-independent
  `MemoryInit()` and `MemoryShutdown()` that allow mulitple calls
  and invoke the underlying allocator only once.

Fixes #3398 — Figure out how QR code parsing can be used safely with the MemoryInit requirement
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants