Attention is currently required from: Cheng-Hao Yang.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
service_provider_timer_.Start(FROM_HERE, kDelayBetweenConnectAttempts, this,
&SensorHalDispatcher::ConnectToServiceManager);
I think we don't need this. The service manager will be initialized in the ash main function[1]. After that, `IsServiceManagerConnected()` will only return false if the service manager is not enabled.
So you just need to call the initialize function after the mojo service manager initialized. See CL:3758176 for example.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #2, Line 16: constexpr char kSensorHalClientServiceName[] = "SensorHalClient";
Please put this into system api: https://source.corp.google.com/chromeos_public/src/platform2/system_api/mojo/service_constants.h;l=1;bpv=0
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu.
2 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #2, Line 16: constexpr char kSensorHalClientServiceName[] = "SensorHalClient";
Please put this into system api: https://source.corp.google. […]
Will do.
service_provider_timer_.Start(FROM_HERE, kDelayBetweenConnectAttempts, this,
&SensorHalDispatcher::ConnectToServiceManager);
I think we don't need this. The service manager will be initialized in the ash main function[1]. […]
Yeah, while in my test, SensorHalDispatcher seems to be constructed before the mojo service manager is initialized.
Any other suggestion?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
service_provider_timer_.Start(FROM_HERE, kDelayBetweenConnectAttempts, this,
&SensorHalDispatcher::ConnectToServiceManager);
Yeah, while in my test, SensorHalDispatcher seems to be constructed before the mojo service manager […]
Why does SensorHalDispatcher need to be created before it? Does others depends on it?
If so, you can add a `RegisterMojoService()` method and call it after service manager is ready. Just like CL:3758176 does.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #2, Line 16: constexpr char kSensorHalClientServiceName[] = "SensorHalClient";
Will do.
Should be updated after CL:3807024
service_provider_timer_.Start(FROM_HERE, kDelayBetweenConnectAttempts, this,
&SensorHalDispatcher::ConnectToServiceManager);
Why does SensorHalDispatcher need to be created before it? Does others depends on it? […]
So it's initialized here [1] with dbus services, as it currently support sensor server and clients via dbus. I'd prefer not to move it now to avoid issues.
Adopting your suggestion. Thanks!
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang.
4 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.h:
Patch Set #4, Line 58: remote
This should be a receiver of an mojo interface.
I thought we have a mojo interface for `SensorHalDispatcher` and the powerd can call it. But seems like these remote were passed through dbus.
Passing remote here could work but not recommended because this break the waiting logic (you need to manually wait the remote to be registered).
Can you add comment to describe that this is a workaround and we should not use remote here?
Or maybe we should migrate iioservice first so powerd can pass the receiver to iioservice directly.
WDYT?
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
I think this can be checked in caller.
if (service_provider_.is_bound()) {
// SensorHalClient service already registered.
return;
}
We can just use CHECK here. This should not be called twice.
Patch Set #4, Line 82: auto* proxy = mojo_service_manager::GetServiceManagerProxy();
I think we can check this in caller and pass this to `RegisterMojoService()` so we don't need to check here (or just use DCHECK here).
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #4, Line 85: service_provider_
You may want to register a disconnect handler to handle the error.
E.g.
```
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR)
<< "Register failed: error: " << error << ", message: " << message;
}));
```
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu.
5 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.h:
Patch Set #4, Line 58: remote
This should be a receiver of an mojo interface. […]
Sure. Adding a comment here to avoid confusion.
Migrating iioservice requires more steps to launch mojo_service_manager. Let's use this CL to unblock your work.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
I think this can be checked in caller.
If it's only used after |mojo_service_manager::CreateConnectionAndPassCloser()|, I guess we don't need to check it at all?
if (service_provider_.is_bound()) {
// SensorHalClient service already registered.
return;
}
We can just use CHECK here. This should not be called twice.
Done
Patch Set #4, Line 82: auto* proxy = mojo_service_manager::GetServiceManagerProxy();
I think we can check this in caller and pass this to `RegisterMojoService()` so we don't need to che […]
Done
Patch Set #4, Line 85: service_provider_
You may want to register a disconnect handler to handle the error. […]
Right, thanks!
I guess we don't need to reconnect when the error occurs?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang.
2 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
If it's only used after |mojo_service_manager::CreateConnectionAndPassCloser()|, I guess we don't ne […]
If the service manager is not enabled by command line option, it will be remained unavailable.
Patch Set #4, Line 85: service_provider_
Right, thanks! […]
Yes because if it is service manager issue, the chrome will just crash when the main interface disconnected.
This will most likely to log the permission error.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu.
2 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
If the service manager is not enabled by command line option, it will be remained unavailable.
In this case, this function [1] will never be run, so I guess it's fine?
Patch Set #4, Line 85: service_provider_
Yes because if it is service manager issue, the chrome will just crash when the main interface disco […]
Ack
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Ryo Hashimoto.
2 comments:
Patchset:
@hashimoto, PTAL and see if you have any concern.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
In this case, this function [1] will never be run, so I guess it's fine? […]
oh, I see. Then how about using DCHECK instead of return?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Ryo Hashimoto.
1 comment:
Patchset:
See CL:3812976. You also need to add and install a policy file to allow Chrome register the SensorHalClient service.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu, Ryo Hashimoto.
2 comments:
Patchset:
See CL:3812976. […]
Yeah, I've already added it here [1]. Please help review the policy file at that CL. (The identity of chrome in my coachz is different from CL:3812976, which is weird to me...)
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
oh, I see. […]
Already done in Patchset 5 [1], only that I did that in the function instead of at the caller. I guess it's better this way?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Ryo Hashimoto.
2 comments:
Patchset:
Yeah, I've already added it here [1]. Please help review the policy file at that CL. […]
See also https://chromium-review.googlesource.com/c/chromium/src/+/3758175/comments/46b5c94f_7fa30613
We decided to organizing the policy the same way as DBus. So we should put the policy file here because the owner is ash-chrome. Please see CL:3812976 for examples.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
if (!mojo_service_manager::IsServiceManagerConnected())
return;
Already done in Patchset 5 [1], only that I did that in the function instead of at the caller. […]
Ack
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu, Ryo Hashimoto.
1 comment:
Patchset:
See also https://chromium-review.googlesource. […]
Ah I see. Thanks for the explanation!
Done.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Chung-Sheng Wu.
6 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.h:
Patch Set #6, Line 60: /*identity*/
Just `identity`.
No need to make this a comment.
Patch Set #6, Line 67: void ConnectToServiceManager();
Looks like this doesn't exist.
Patch Set #6, Line 80: service_provider_
How about something like receiver_?
The name service_provider_ is confusing when this object is actually a receiver, and |this| actually implements ServiceProvider.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
I don't understand why this is named as kChromiumSensorHalClient.
What you're registering is a service, not a client.
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
Will this ever be useful?
Will there be anything we can do when the service manager dies?
Patch Set #6, Line 110: /*identity*
Just `identity`.
No need to make this a comment.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
I don't understand why this is named as kChromiumSensorHalClient. […]
I had an offline sync with Harvey. The SensorHalClient is the mojo interface name. It is for binding the mojo pipe from clients.
Note that the ServiceProvider of this receive a mojo::Remote not a mojo::Receiver because of the original design of iioservice.
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
Will this ever be useful? […]
This can log the permission error.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ryo Hashimoto.
6 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.h:
Patch Set #6, Line 60: /*identity*/
Just `identity`. […]
Thanks!
Patch Set #6, Line 67: void ConnectToServiceManager();
Looks like this doesn't exist.
Right, sorry. Removed.
Patch Set #6, Line 80: service_provider_
How about something like receiver_? […]
Right, thanks!
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
I don't understand why this is named as kChromiumSensorHalClient. […]
As all other service names are named like this, Chung-Sheng recommended me [1] to do the same.
[1]: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3807024/comments/032e54a0_9992b494
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
Will this ever be useful? […]
Chung-Sheng recommended me [1] to add this, just to provide an error log. Mojo service manager only restarts when ui restarts as well, so we shouldn't need to do anything here.
[1]: https://chromium-review.googlesource.com/c/chromium/src/+/3759053/comments/2c5f347f_6835198a
Patch Set #6, Line 110: /*identity*
Just `identity`. […]
Thanks!
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Chung-Sheng Wu.
2 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
Sorry for being unclear. I don't think it makes sense to have a word "client" in a service name. It's just confusing. The fact that the existing interface already contains a word client in it doesn't justify having a confusing service name.
Note that the ServiceProvider of this receive a mojo::Remote not a mojo::Receiver because of the original design of iioservice.
I also don't understand why you're doing this when you can just receive a Receiver and bind it to chromeos.sensors.mojom.SensorService.
Furthermore, I'm even more confused by the fact that you're adding this service when you can just register service in iioservice. What is the point of having this service registered?
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
This can log the permission error.
Shouldn't this be logged by the service manager?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
I also don't understand why you're doing this when you can just receive a Receiver and bind it to chromeos.sensors.mojom.SensorService.
Adding |kChromiumSensorHalClient| as a service is a workaround during the migration, especially needed for powerd, which doesn't restart with ash-chrome.
Eventually, yes, we only need a service like "SensorService" owned by iioservice, and all the other sensor clients requesting it via mojo service manager. While during the workaround, we need to support SensorHalDispatcher before updating iioservice and all the sensor clients.
We can also migrate iioservice to own two services "SensorService" and "SensorHalServer" (to support SensorHalDispatcher and the current sensor clients. Will be removed in the end), and let powerd request "SensorService", while it requires more CLs to unblock Chung-Sheng's work. (It's still needed nonetheless during the migration though)
WDYT?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Ryo Hashimoto.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
> This can log the permission error. […]
Currently, no. Do you think it should be logged by the service manager instead of each client?
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #9, Line 74: IsServiceManagerConnected
After CL:3813082 this is renamed to `IsServiceManagerBound()`
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
> I also don't understand why you're doing this when you can just receive a Receiver and bind it to […]
Even if you need to go through this workaround state, could you at least come up with:
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
Currently, no. […]
If this is needed by all clients, then I think it makes sense to do that in the service manager instead.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Chung-Sheng Wu, Ryo Hashimoto.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
A design which doesn't involve sending mojo::Remote to a ServiceProvider. I don't think it's a good idea to make the mojo service manager's security model complicated.
Either "SensorHalClient" or "SensorHalServer", with the current mojo interfaces, I think we have to send mojo::Remote to support SensorHalDispatcher during the workaround state, unless we introduce new mojo interfaces/methods. Like chromeos.sensors.mojom.SensorHalDispatcher (iioservice and sensor clients hold the remote side, and Chromium::SensorHalDispatcher holds the receiver side), just like camera does [1].
Do you really think this is needed, just for the workaround state? Or any other suggestion?
Thanks for looking into this :)
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Ryo Hashimoto.
2 comments:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
> A design which doesn't involve sending mojo::Remote to a ServiceProvider. […]
Maybe we should go with the way we have discussed.
1. Powerd connects to iioservice directly, obtain the SensorService.
2. iioservice export SensorHalServer and let ash-chrome connect to it (which require some refactor in SensorHalDispethcer.)
We need 1. to remove the respawn of service manager. We need 2. so iioservice can connect to service manager for 1.
Because of the design of iioservice, if we don't want to pass mojo::Remote, we need to change the direction of connection from iio->ash to ash->iio.
service_provider_.set_disconnect_with_reason_handler(
base::BindOnce([](uint32_t error, const std::string& message) {
LOG(ERROR) << "Register failed: error: " << error
<< ", message: " << message;
}));
If this is needed by all clients, then I think it makes sense to do that in the service manager inst […]
Ok, I will add it to the service manager. We can remove this here.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Ryo Hashimoto.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
Maybe we should go with the way we have discussed. […]
Hi Ryo,
I just found a way to have a better workaround path, and have confirmed with Chung-Sheng that it should work: As SensorHalDispatcher can be a sensor client proxy, which calls |chromeos.mojo_service_manager.mojom::ServiceManager::Request("SensorService", ...)| (instead of |chromeos.sensors.mojom.SensorHalServer.CreateChannel|) to bootstrap a SensorService mojo pipe between iioservice and a sensor client when a sensor client is registered (via chromeos.sensors.mojom.SensorHalClient) to SensorHalDispatcher.
In other words, SensorHalDispatcher will request "SensorService" service multiple times.
This way, we don't need to introduce any new mojo interface/method, nor do we need to add workaround services (like "SensorHalServer" or "SensorHalClient") in mojo_service_manager, and it also doesn't involve sending mojo::Remote to a ServiceProvider.
Detailed steps:
1. Implement the logic to support both ways to send mojom.SensorService mojo pipes to iioservice in SensorHalDispatcher (with help of [1] & [2]).
2. Let iioservice be bootstrapped by mojo_service_manager, and register "SensorService" service. After this, SensorHalDispatcher will be able to use the "SensorService" service to send mojom.SensorService mojo pipes.
3. Migrate each sensor client to be bootstrapped by mojo_service_manager (CrOS daemons) / utilize existing chromeos.mojo_service_manager.mojom::ServiceManager mojo channel (Chromium components). Powerd will be the first to unblock Chung-Sheng's work.
4. Remove the old logic in SensorHalDispatcher that supports SensorHalServer and SensorHalClient.
5. Remove both mojo interfaces: chromeos.sensors.mojom.SensorHalServer & chromeos.sensors.mojom.SensorHalClient.
WDYT, Ryo?
[1]: https://source.corp.google.com/chromeos_public/src/platform2/mojo_service_manager/lib/mojom/service_manager.mojom;l=38
[2]: https://source.corp.google.com/chromeos_public/src/platform2/mojo_service_manager/lib/mojom/service_manager.mojom;l=44
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Cheng-Hao Yang, Chung-Sheng Wu.
1 comment:
File chromeos/components/sensors/ash/sensor_hal_dispatcher.cc:
Patch Set #6, Line 78: kChromiumSensorHalClient
Hi Ryo, […]
The plan sgtm. Thanks!
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.
Cheng-Hao Yang abandoned this change.
To view, visit change 3759053. To unsubscribe, or for help writing mail filters, visit settings.