Migrate hinge angle injection to sensors simulator#2643
Conversation
GoogleCuttlefishTesterBot
removed
the
kokoro:force-run
| auto sensors_data = sensors_simulator_.GetSensorsData(mask); | ||
| std::lock_guard<std::mutex> lock(report_mtx_); | ||
| auto result = UpdateSensorsHal(sensors_data, data_channel_, mask); |
There was a problem hiding this comment.
I see that this was moved from data_reporter_thread_, but note abseil.io/tips/232
Use type deduction only if it makes the code clearer to readers who aren’t familiar with the project, or if it makes the code safer. Do not use it merely to avoid the inconvenience of writing an explicit type.
There was a problem hiding this comment.
Agreed with making sensors_data explicit (std::string) above, the type isn't obvious from GetSensorsData, but I kept result as auto since auto result = …; if (!result.ok()) for Result<void> is the idiom used throughout this file and the wider codebase. Happy to spell it out too if you'd rather go the other way.
| if (on_change_callback_) { | ||
| on_change_callback_(1 << kHingeAngle0Id); | ||
| } |
There was a problem hiding this comment.
Is there anything else that should also invoke this new callback? SetMotion also affects state and seems relevant to something called "on change callback".
There was a problem hiding this comment.
Good catch, you're right it's odd for a "changed" callback to fire only from SetHingeAngle.
Context: the HAL proxy splits sensors into continuous (kContinuousModeSensors, streamed to the guest every cycle) and on-change (kOnChangeSensors, currently just the hinge angle). Motion sensors are continuous, so the polling loop already reports them regularly; there's nothing to push when SetMotion runs.
The catch is that answering "no, SetMotion shouldn't fire it" means teaching the simulator which sensors are continuous vs on-change. But that's the proxy's concept, and leaking it into the simulator is exactly what made this confusing in the first place.
So I made the callback generic instead: both SetMotion and SetHingeAngle just report whatever they changed, and the subscriber passes a mask of what it cares about. The proxy subscribes with kOnChangeSensors, so motion updates get filtered out there and the continuous/on-change policy stays entirely in the proxy where it belongs.
Do you think it looks cleaner now?
| element.dataset.adb = true; | ||
| } | ||
| } | ||
| element.dataset.adb = true; |
There was a problem hiding this comment.
Should this still be here? If I understand correctly, the code before amounted to something like "if the device state includes hinge_angle, make sure adb is connected."
As written this commit changes it to "always connect to adb," but I would expect "the hinge angle is not sent via adb" to imply "adb no longer matters for sensors, don't set or unset the adb connection requirement."
I may be misunderstanding what the .adb variable is for. FYI @jemoreira
There was a problem hiding this comment.
From the change perspective this makes sense, I'm agree.
However, after checking the getCustomDeviceStateButtonCb implementation, it looks to me like any action on that panel triggers adbShell('cmd device_state state reset'), meaning ADB is executed regardless of whether we change the hinge angle, the lid, or anything else, so I assume the entire panel should be disabled if ADB isn't available.
Do you think I'm missing something?
The cuttlefish_sensor_injection tool does not work with goldfish multihal sensors, meaning hinge angle changes currently have no effect To fix this, redirect WebRTC hinge angle updates to the sensors simulator instead. Test: hinge angle is updated from web ui Bug: 517370262 Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
3 participants
The cuttlefish_sensor_injection tool does not work with goldfish multihal sensors, meaning hinge angle changes currently have no effect
To fix this, redirect WebRTC hinge angle updates to the sensors simulator instead.
Test: hinge angle is updated from web ui
Bug: 517370262