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

Stability enhancemets #7272

Merged
merged 4 commits into from
Sep 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion common/output-model.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ void output_model::thread_loop()
to_string() << "Failed to fetch firmware logs: " << ex.what());
}
}
std::this_thread::sleep_for(std::chrono::milliseconds(10));
// FW define the logs polling intervals to be no less than 100msec to cope with limited resources.
// At the same time 100 msec should guarantee no log drops
std::this_thread::sleep_for(std::chrono::milliseconds(100));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/archive.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ namespace librealsense
void keep() override
{
auto frames = get_frames();
for (int i = 0; i < get_embedded_frames_count(); i++)
for (size_t i = 0; i < get_embedded_frames_count(); i++)
if (frames[i]) frames[i]->keep();
frame::keep();
}
Expand Down
2 changes: 1 addition & 1 deletion src/ds5/ds5-device.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ namespace librealsense
raw_depth_sensor.get_notifications_processor(),
std::unique_ptr<notification_decoder>(new ds5_notification_decoder())));

depth_sensor.register_option(RS2_OPTION_ERROR_POLLING_ENABLED, std::make_shared<polling_errors_disable>(_polling_error_handler.get()));
depth_sensor.register_option(RS2_OPTION_ERROR_POLLING_ENABLED, std::make_shared<polling_errors_disable>(_polling_error_handler));

depth_sensor.register_option(RS2_OPTION_ASIC_TEMPERATURE,
std::make_shared<asic_and_projector_temperature_options>(raw_depth_sensor,
Expand Down
9 changes: 7 additions & 2 deletions src/hw-monitor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ namespace librealsense
return _locked_transfer->send_receive(data);
}

std::vector<uint8_t> hw_monitor::send( command cmd, hwmon_response * p_response ) const
std::vector<uint8_t> hw_monitor::send( command cmd, hwmon_response * p_response , bool locked_transfer) const
{
hwmon_cmd newCommand(cmd);
auto opCodeXmit = static_cast<uint32_t>(newCommand.cmd);
Expand All @@ -136,6 +136,11 @@ namespace librealsense
details.sendCommandData.data(),
details.sizeOfSendCommandData);

if (locked_transfer)
{
return _locked_transfer->send_receive({ details.sendCommandData.begin(),details.sendCommandData.end()});
}

send_hw_monitor_command(details);

// Error/exit conditions
Expand All @@ -150,7 +155,7 @@ namespace librealsense

// endian?
auto opCodeAsUint32 = pack(details.receivedOpcode[3], details.receivedOpcode[2],
details.receivedOpcode[1], details.receivedOpcode[0]);
details.receivedOpcode[1], details.receivedOpcode[0]);
if (opCodeAsUint32 != opCodeXmit)
{
auto err_type = static_cast<hwmon_response>(opCodeAsUint32);
Expand Down
2 changes: 1 addition & 1 deletion src/hw-monitor.h
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ namespace librealsense
{}

std::vector<uint8_t> send(std::vector<uint8_t> data) const;
std::vector<uint8_t> send( command cmd, hwmon_response * = nullptr ) const;
std::vector<uint8_t> send( command cmd, hwmon_response * = nullptr, bool locked_transfer = false ) const;
void get_gvd(size_t sz, unsigned char* gvd, uint8_t gvd_cmd) const;
static std::string get_firmware_version_string(const std::vector<uint8_t>& buff, size_t index, size_t length = 4);
static std::string get_module_serial_string(const std::vector<uint8_t>& buff, size_t index, size_t length = 6);
Expand Down
2 changes: 1 addition & 1 deletion src/l500/l500-depth.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ namespace librealsense
raw_depth_sensor.get_notifications_processor(),
std::unique_ptr<notification_decoder>(new l500_notification_decoder())));

depth_sensor.register_option(RS2_OPTION_ERROR_POLLING_ENABLED, std::make_shared<polling_errors_disable>(_polling_error_handler.get()));
depth_sensor.register_option(RS2_OPTION_ERROR_POLLING_ENABLED, std::make_shared<polling_errors_disable>(_polling_error_handler));

// attributes of md_capture_timing
auto md_prop_offset = offsetof(metadata_raw, mode) +
Expand Down
3 changes: 2 additions & 1 deletion src/l500/l500-device.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,8 @@ namespace librealsense
throw wrong_api_call_sequence_exception("_hw_monitor is not initialized yet");

command cmd(ivcam2::fw_cmd::MRD, ivcam2::REGISTER_CLOCK_0, ivcam2::REGISTER_CLOCK_0 + 4);
auto res = _hw_monitor->send(cmd);
// Redirect HW Monitor commands to used atomic (UVC) transfers for faster transactions and transfer integrity
auto res = _hw_monitor->send(cmd, nullptr, true);
maloel marked this conversation as resolved.
Show resolved Hide resolved

if (res.size() < sizeof(uint32_t))
{
Expand Down
9 changes: 7 additions & 2 deletions src/option.cpp
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
// License: Apache 2.0. See LICENSE file in root directory.
// Copyright(c) 2015 Intel Corporation. All Rights Reserved.

#include "option.h"
#include "sensor.h"
#include "error-handling.h"


bool librealsense::option_base::is_valid(float value) const
{
Expand Down Expand Up @@ -142,6 +141,12 @@ std::vector<uint8_t> librealsense::command_transfer_over_xu::send_receive(const
});
}

librealsense::polling_errors_disable::~polling_errors_disable()
{
if (_polling_error_handler)
_polling_error_handler->stop();
}

void librealsense::polling_errors_disable::set(float value)
{
if (value < 0)
Expand Down
10 changes: 6 additions & 4 deletions src/option.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include "sensor.h"
#include "core/streaming.h"
#include "command_transfer.h"

#include "error-handling.h"
#include <chrono>
#include <memory>
#include <vector>
Expand Down Expand Up @@ -452,10 +452,12 @@ namespace librealsense
class polling_errors_disable : public option
{
public:
polling_errors_disable(polling_error_handler* handler)
: _polling_error_handler(handler), _value(1)
polling_errors_disable(std::unique_ptr<polling_error_handler>& handler)
: _polling_error_handler(std::move(handler)), _value(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I like this: why not make it a shared_ptr<> instead, then we don't run into a situation where the value is now null but there really still is a handler?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fix addresses only the issue of the polling handler in 'orphan' mode after device is (forcibly) disconnected.
In that case the device is indeed is 0xdeadbeef but the handler remained active and could fail on dereferencing invalid pointer.
With this design the handler will remain intact, and after device disconnects it will stop the active object in dtor.
So the polling events will be handled properly during the period between device disconnect and the dtor call

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also recognize more things to improve in the design of active objects lifetime control but this will require a separate session

{}

~polling_errors_disable();

void set(float value) override;

float query() const override;
Expand All @@ -473,7 +475,7 @@ namespace librealsense
_recording_function = record_action;
}
private:
polling_error_handler* _polling_error_handler;
std::unique_ptr<polling_error_handler> _polling_error_handler;
float _value;
std::function<void(const option&)> _recording_function = [](const option&) {};
};
Expand Down
2 changes: 1 addition & 1 deletion src/proc/synthetic-stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ namespace librealsense
// will only read from the currently selected
// block, setting an option will propogate
// to all blocks in the group
for (int i = 0; i < _parent->_processing_blocks.size(); i++)
for (size_t i = 0; i < _parent->_processing_blocks.size(); i++)
{
if (_parent->_processing_blocks[i]->supports_option(_opt))
{
Expand Down