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

Increase the number of read/write client/handler to 4 #7967

Conversation

yunhanw-google
Copy link
Contributor

@yunhanw-google yunhanw-google commented Jun 28, 2021

Problem

Similar to Command, we also need increase the number of read/write client/handler to 4 for multiple read/write situations

Change overview

increase the number of read/write client/handler to 4

Testing

Manual compilation and run device controller test with song's IM read with ember integration

@todo
Copy link

todo bot commented Jun 28, 2021

Make number of command/read/write client/handler configurable

// TODO: Make number of command/read/write client/handler configurable
#define CHIP_MAX_NUM_COMMAND_HANDLER 4
#define CHIP_MAX_NUM_COMMAND_SENDER 4
#define CHIP_MAX_NUM_READ_CLIENT 4
#define CHIP_MAX_NUM_READ_HANDLER 4
#define CHIP_MAX_REPORTS_IN_FLIGHT 4
#define IM_SERVER_MAX_NUM_PATH_GROUPS 8
#define CHIP_MAX_NUM_WRITE_CLIENT 4
#define CHIP_MAX_NUM_WRITE_HANDLER 4
namespace chip {


This comment was generated by todo based on a TODO comment in 27ffdbd in #7967. cc @yunhanw-google.

@woody-apple
Copy link
Contributor

Why did we choose 4? Is there a better path forward here for constrained devices? @bzbarsky-apple ?

@yunhanw-google
Copy link
Contributor Author

Why did we choose 4? Is there a better path forward here for constrained devices? @bzbarsky-apple ?

@bzbarsky-apple @woody-apple I think we have clear TODO "Make number of command/read/write client/handler configurable"

@yunhanw-google
Copy link
Contributor Author

Why did we choose 4? Is there a better path forward here for constrained devices? @bzbarsky-apple ?

#8006

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

And just to be clear: 4 clients is totally inadequate for any sort of resident/hub/etc device. We need a better allocation strategy here.

On the other hand, having 4 handlers is total overkill because all of our handlers return sychronously afaict, so I don't think we ever use more than 1.

@bzbarsky-apple
Copy link
Contributor

Is there a better path forward here for constrained devices?

The right numbers are something like:

  • ReadHandler/WriteHandler/CommandHandler: 1, possibly allocated on the stack, not living in bss. Questionable whether these objects should even exist in their current form, especially CommandHandler (which has a bunch of unused members, etc).
  • Constrained device ReadClient/WriteClient/CommandClient: Depends on the device, between 0 and 1.
  • Unconstrained device ReadClient/WriteClient/CommandClient: infinite, bounded by available RAM (and maybe not even that, for devices with swap).

This PR is sort of no worse than the state before; Sagar's attempts to use this on iOS will still be broken and he will need to bump these numbers to much larger values to get anything usable, our CI will still work because it does one thing at a time.... But it's not really any better than the state before either. We should stop wasting our time twiddling these constants and actually solve the real problems.

@woody-apple
Copy link
Contributor

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.

5 participants