Skip to content

enable usb weak host workaround to improve android stability #7205

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

Merged
merged 7 commits into from
Sep 13, 2020

Conversation

gwen2018
Copy link
Contributor

@gwen2018 gwen2018 commented Aug 27, 2020

Enable L515 android stability workaround. Current usb buffer settings in device require large number of small usb transactions from device to usb host controller, some hosts with relatively weak host cannot handle this many transactions quickly, results in stability issues, camera disappear from os enumeration or one of the streams dies during stream, etc. This workaround uses larger buffer settings to reduce the number of transaction and improve performance and stability.

Workaround is enabled by default for all android platforms. Requires additional firmware command support in new firmware release.

Tracked on: RS5-8011

@gwen2018
Copy link
Contributor Author

@maloel thanks for review feedback. hope the changes addressed concerns.

@maloel
Copy link
Contributor

maloel commented Aug 29, 2020

Hi @gwen2018
The changes are fine. I'm a little hesitant about the FW version... can we delay the merge of this until they give a proper version? Or do you need it in right away?

@gwen2018
Copy link
Contributor Author

Hi @gwen2018
The changes are fine. I'm a little hesitant about the FW version... can we delay the merge of this until they give a proper version? Or do you need it in right away?

Thanks. I will check with firmware team.

@@ -96,6 +96,7 @@ extern "C" {
RS2_OPTION_THERMAL_COMPENSATION, /**< Depth Thermal Compensation for selected D400 SKUs */
RS2_OPTION_TRIGGER_CAMERA_ACCURACY_HEALTH, /**< Enable depth & color frame sync with periodic calibration for proper alignment */
RS2_OPTION_RESET_CAMERA_ACCURACY_HEALTH,
RS2_OPTION_ENABLE_WEAK_USB_HOST_WA, /**< Enable workaround for weak USB hosts to improve performance and stability */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls rename and refactor the option to be rs2_option_trb_size so that it will read/set the TRB size (units TBD) - it is not platform specfic and therefore may be utilized in additional scenarios.
You can set an enum for the supported RGB sizes in order to enforce strict (and validated) values


auto usb_perf_enabled = get_option(RS2_OPTION_ENABLE_WEAK_USB_HOST_WA).query();

if (usb_perf_enabled)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be encapsulated into opt->set

…t on android and default settings for other platforms, user has the option to change
@gwen2018
Copy link
Contributor Author

@ev-mp thanks for the review. refactored the option, included your suggestion, name is host performance mode, because it will be a generic option for future use as well, this time, only include the TRB settings.

at launch, on android platforms, low performance mode is turned on and larger TRB settings, on other platforms (windows, linux, etc.), device default setting is used at launch. user has the option to change the option later after viewer launch.

The option is only for L515, the conditional compilation is minimized (only the launch performance mode).

also added settings for the 4th endpoint (confidence stream). now the settings are complete.

@@ -374,6 +392,67 @@ namespace librealsense

void l500_depth_sensor::start(frame_callback_ptr callback)
{
if (supports_option(RS2_OPTION_HOST_PERFORMANCE))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about encapsulating the below code into opt->set(..) ?
The TRB settings are customized per sensor and can be passed into the option at ctor level (where sensor->register_option( occrs), so this could can become:
if supports(TRB)
trb_opt-set(TRB_MOD)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the suggestion, looked into it, will implement tomorrow. if need to make the build, please go ahead and take what current implementation. can do another PR for this part of the change once it's tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, we'll merge the current implementation so that refactoring will be moved to the follow-on task.
@gwen2018 , please open a new PR for the remaining work.

@@ -96,6 +96,7 @@ extern "C" {
RS2_OPTION_THERMAL_COMPENSATION, /**< Depth Thermal Compensation for selected D400 SKUs */
RS2_OPTION_TRIGGER_CAMERA_ACCURACY_HEALTH, /**< Enable depth & color frame sync with periodic calibration for proper alignment */
RS2_OPTION_RESET_CAMERA_ACCURACY_HEALTH,
RS2_OPTION_HOST_PERFORMANCE, /**< Set host performance mode to optimize device settings so host can keep up with workload, for example, USB transaction granularity, setting option to low performance host leads to larger USB transaction size and reduced number of transactions which improves performance and stability if host is relatively weak as compared to workload */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure the option and enum are propagated into python (+pybackend), C#, Java and Matlab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added to python and java for now, but not c# and matlab yet.

Copy link
Collaborator

@ev-mp ev-mp left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the in-depth analysis!

@ev-mp ev-mp merged commit e229fdb into IntelRealSense:development Sep 13, 2020
@gwen2018 gwen2018 deleted the l515_android_stability_wa branch February 4, 2025 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants