-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fw logs and terminal parser api fix, wrappers android and C# #6743
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For review
int result = rs2_get_fw_log(reinterpret_cast<rs2_device*>(fw_logger_handle), &log_msg, &e); | ||
handle_error(env, e); | ||
|
||
return (jlong)log_msg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffers should be released with rs2_delete_fw_log_message
.
Probably RAII?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Java class FwLogMsg which gets this handle - which is actually the pointer of the native allocation - inherits from LrsClass, which inherits from AutoClosable. This ensures that if the instance has been allocated within a "try" call, the close method of the class will be closed at the end of the block after the "try" call.
The rs2_delete_fw_log_message method is called in the close method of the FwLogMsg.
src/android/jni/fw_logger.cpp
Outdated
|
||
int result = rs2_get_flash_log(reinterpret_cast<rs2_device*>(fw_logger_handle), &log_msg, &e); | ||
if (result == 0){ | ||
e = rs2_create_error("No more flash logs in flash", "dummy_name", "dummy_args", RS2_EXCEPTION_TYPE_UNKNOWN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"No more flash logs on flash"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected
try(RsContext ctx = new RsContext()) { | ||
try (DeviceList devices = ctx.queryDevices()) { | ||
if (devices.getDeviceCount() > 0) { | ||
try (Device device = devices.createDevice(0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will pick-up the first device found. Add device selection for multi-cam scenario
try(RsContext ctx = new RsContext()){ | ||
try(DeviceList devices = ctx.queryDevices()) { | ||
if (devices.getDeviceCount() > 0) { | ||
try (Device device = devices.createDevice(0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
String fw_logging_file_path = sharedPref.getString(getString(R.string.fw_logging_file_path), ""); | ||
|
||
|
||
if (fw_logging_enabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happen if resumeFwLogger
is called twice, what will happen to the original thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is not part of the API, this is part of the application.
It is not called twice, I don't think I have to check this issue.
… in FwLogsThread for unique device
4a7d1b6
to
e36e49e
Compare
src/android/jni/fw_logger.cpp
Outdated
|
||
int result = rs2_get_flash_log(reinterpret_cast<rs2_device*>(fw_logger_handle), log_msg, &e); | ||
if (result == 0){ | ||
e = rs2_create_error("No more logs in flash", "dummy_name", "dummy_args", RS2_EXCEPTION_TYPE_UNKNOWN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there is need for an error? Returning zero messages is not an exceptional UC.
if (device != null) { | ||
try (final FwLogger fwLoggerDevice = device.as(Extension.FW_LOGGER)) { | ||
mFwLoggerDevice = fwLoggerDevice; | ||
if (mIsParsingFileInitialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be if not initialized
?
} else { | ||
logReceived = logMsg.getTimestamp() + " " + | ||
logMsg.getSeverityStr() + " "; | ||
byte[] buffer = new byte[512]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify reference to c++ code/constant used for hard-coded value
if(device != null) { | ||
try(final FwLogger fwLoggerDevice = device.as(Extension.FW_LOGGER)){ | ||
mFwLoggerDevice = fwLoggerDevice; | ||
if (mIsParsingFileInitialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR triggered by jira ticket DSO-15212.
Fw logs in the android wrapper are now based on the fw logs API which has been recently implemented.