Skip to content

Add support for 8 bit thermal camera image format#92

Merged
nkoenig merged 8 commits intoign-sensors4from
thermal_8bit
Feb 10, 2021
Merged

Add support for 8 bit thermal camera image format#92
nkoenig merged 8 commits intoign-sensors4from
thermal_8bit

Conversation

@iche033
Copy link
Contributor

@iche033 iche033 commented Feb 5, 2021

Summary

Depends on gazebosim/gz-rendering#235

Adds the necessary hooks to set ign-rendering thermal camera sensor to 8 bit image format and publish the generated image data.

Note that ign-rendering still returns uint16_t in the image callback but it actually contains 8 bit data. This was done to avoid breaking ABI. However, the image that's published by ign-sensors will have the correct format in a 8 bit unsigned char array

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
    Note to maintainers: Remember to use Squash-Merge

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 requested a review from adlarkin February 5, 2021 09:04
@iche033 iche033 added enhancement New feature or request needs upstream release Blocked by a release of an upstream library 🔮 dome Ignition Dome labels Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #92 (e87e030) into ign-sensors4 (46b320f) will increase coverage by 0.12%.
The diff coverage is 87.87%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-sensors4      #92      +/-   ##
================================================
+ Coverage         76.00%   76.13%   +0.12%     
================================================
  Files                23       23              
  Lines              2359     2388      +29     
================================================
+ Hits               1793     1818      +25     
- Misses              566      570       +4     
Impacted Files Coverage Δ
src/ThermalCameraSensor.cc 60.00% <87.50%> (+3.29%) ⬆️
src/Sensor.cc 87.90% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46b320f...c6b39cc. Read the comment docs.

@iche033 iche033 requested a review from ahcorde February 8, 2021 22:55
@adlarkin
Copy link
Contributor

adlarkin commented Feb 9, 2021

There are a few test failures on macOS, but I don't think they're related to this PR. If that's true, then everything else looks good to me, assuming that defaulting to 16 bit is okay.

@iche033
Copy link
Contributor Author

iche033 commented Feb 9, 2021

assuming that defaulting to 16 bit is okay.

yes that also preserves existing behavior

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Something is broken on the macOS github actions. It's related with OGRE2

[Err] [Ogre2RenderEngine.cc:436] Unable to find Ogre Plugin[/usr/local/opt/ogre2.1/lib/OGRE-2.1/RenderSystem_GL3Plus]. Rendering will not be possible.Make sure you have installed OGRE properly.
	 13 - INTEGRATION_camera_plugin (SEGFAULT)
	 15 - INTEGRATION_depth_camera_plugin (SEGFAULT)
	 17 - INTEGRATION_gpu_lidar_sensor_plugin (SEGFAULT)
	 19 - INTEGRATION_rgbd_camera_plugin (SEGFAULT)

Otherwise LGTM

Nate Koenig added 2 commits February 10, 2021 08:43
Signed-off-by: Nate Koenig <nate@openrobotics.org>
@adlarkin
Copy link
Contributor

adlarkin commented Feb 10, 2021

When running the integration test for an invalid configuration in gazebosim/gz-sim#614, I notice the following output:

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, range = [0, 255*3] K
[Wrn] [RenderUtil.cc:810] Unable to set thermal camera temperature linear resolution. Value must be greater than 0. Using the default value: 3. 
[Wrn] [RenderUtil.cc:823] Unable to set thermal camera temperature range.Max temperature must be greater or equal to min. Using the default values : [-inf, inf].

It's confusing to me to see two different ranges. I am assuming that [0, 255*3] K is the correct range. Should we modify this warning output? If so, I think the modifications should be made in ign-gazebo (the correct output is coming from this PR).

cc @iche033

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

When running the integration test for an invalid configuration in ignitionrobotics/ign-gazebo#614, I notice the following output:

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, range = [0, 255*3] K
[Wrn] [RenderUtil.cc:810] Unable to set thermal camera temperature linear resolution. Value must be greater than 0. Using the default value: 3. 
[Wrn] [RenderUtil.cc:823] Unable to set thermal camera temperature range.Max temperature must be greater or equal to min. Using the default values : [-inf, inf].

It's confusing to me to see two different ranges. I am assuming that [0, 255*3] K is the correct range. Should we modify this warning output? If so, I think the modifications should be made in ign-gazebo (the correct output is coming from this PR).

cc @iche033

the thermal_system integration test on gazebosim/gz-sim#614 works for me and I don't see the warning messages you posted. Is there something else I should test?

@adlarkin
Copy link
Contributor

the thermal_system integration test on ignitionrobotics/ign-gazebo#614 works for me and I don't see the warning messages you posted. Is there something else I should test?

There are now two thermal integration tests: INTEGRATION_thermal_system (this checks object temperatures) and INTEGRATION_thermal_sensor_system (this checks thermal camera configurations). INTEGRATION_thermal_sensor_system is the new test added in gazebosim/gz-sim#614, and is the one that would produce these warnings (in particular, the ThermalSensorSystemInvalidConfig test). Did you try running the new INTEGRATION_thermal_sensor_system test?

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

The warning messages look correct to me. There are multiple problems with the thermal_invalid.sdf file. The first two warning message refer to the incorrect parameter, and the third warning message refers to the incorrect <max_temp> value.

@adlarkin
Copy link
Contributor

The thing that seems confusing to me is that the first warning indicates that the camera's temperature range is between [0, 255*3] K, but the third warning makes it seem like the camera's temperature range will be between [-inf, inf], which contradicts the information given in the first warning. Does that make sense, or am I misunderstanding something?

@nkoenig
Copy link
Contributor

nkoenig commented Feb 10, 2021

The thing that seems confusing to me is that the first warning indicates that the camera's temperature range is between [0, 255*3] K, but the third warning makes it seem like the camera's temperature range will be between [-inf, inf], which contradicts the information given in the first warning. Does that make sense, or am I misunderstanding something?

Maybe the warning messages could be reworded. The third warning says the that min and max ranges of the camera are between [-inf and inf]. These are ranges that the camera can detect.

The first warning message is about the resolution of the output. My understanding is that there is a difference between what the camera can detect and the data that is produced.

@adlarkin
Copy link
Contributor

My understanding is that there is a difference between what the camera can detect and the data that is produced.

That makes sense. Okay, what if I change the first warning to this (notice the addition of the word output):

[Wrn] [ThermalCameraSensor.cc:275] 8 bit thermal camera image format selected. The temperature linear resolution needs to be higher than 1.0. Defaulting to 3.0, output range = [0, 255*3] K

I think the other warnings are fine as-is.

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin
Copy link
Contributor

Okay, what if I change the first warning to this (notice the addition of the word output)

Made the change in c6b39cc, I think we can merge this.

@nkoenig nkoenig merged commit a9517f8 into ign-sensors4 Feb 10, 2021
@nkoenig nkoenig deleted the thermal_8bit branch February 10, 2021 18:08
@iche033
Copy link
Contributor Author

iche033 commented Feb 10, 2021

a little late but this sums it up:

My understanding is that there is a difference between what the camera can detect and the data that is produced.

  • output range = [0, 255*3] K: this the range of possible values that can be generated by an 8 bit camera with a resolution of 3.
  • [-inf, inf]: are the user specified min and max temperature values.

e.g. user can limit the max temp produced by the camera by specifying something like [0, 200]. If the user specifies something like [0, 800], the max reading will be the min(800, 255*3), which is 765.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔮 dome Ignition Dome enhancement New feature or request needs upstream release Blocked by a release of an upstream library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants