feat: extend discovery for realsense cameras by ChristianDarr-personal · Pull Request #262 · edgexfoundry/device-usb-camera

ChristianDarr-personal · 2023-06-21T19:35:25Z

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?)
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?)

Testing Instructions

Make sure a realsense camera is plugged into your device.
Use v4l2-ctl --list-devices to check that the device is recognized by your system and to check the paths of the device.
Build the usb device service using make docker.
Run the edgex core services along with the device service.
Check the discovered devices (http://0.0.0.0:59881/api/v3/device/service/name/device-usb-camera), make sure that the paths match. Note that only the 1sat, 3rd and 5th paths are saved in the device, as these are the only streaming paths.
Test for rediscovery by deleting the device or restarting the service, etc.
Make sure the paths are updated if ports are changed.

New Dependency Instructions (If applicable)

Signed-off-by: Darr, Christian <christian.darr@intel.com>

codecov-commenter · 2023-06-21T20:37:44Z

Codecov Report

Merging #262 (c54c24d) into main (c5a4cd9) will increase coverage by 2.04%.
The diff coverage is 13.86%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff            @@
##            main    #262      +/-   ##
========================================
+ Coverage   2.38%   4.43%   +2.04%     
========================================
  Files          8       8              
  Lines        879     925      +46     
========================================
+ Hits          21      41      +20     
- Misses       858     883      +25     
- Partials       0       1       +1

Impacted Files	Coverage Δ
internal/driver/device.go	`11.42% <ø> (ø)`
internal/driver/routes.go	`0.00% <0.00%> (ø)`
internal/driver/driver.go	`3.48% <14.00%> (+3.48%)`	⬆️

ajcasagrande

Added some comments to help direct you in possible alternative approaches. Work with @presatish and use best judgement to proceed how you see fit.

cmd/res/provisionwatchers/generic.provision.watcher.yaml

internal/driver/driver.go

ajcasagrande · 2023-06-21T23:23:01Z

internal/driver/device.go

 	lc                          logger.LoggingClient
 	name                        string
-	path                        string
+	paths                       []interface{}


we might want to make these be a string array for simplicity, as i believe this is an internal representation

So that's what I tried originally, but it kept panicking with type errors. This was the solution I settled on, but I will keep on trying to fix those errors

internal/driver/driver.go

ajcasagrande · 2023-06-21T23:26:39Z

internal/driver/driver.go

+// If there is a mismatch between them, scan all paths to find the matching device and update the existing device with the correct path.
+func (d *Driver) RefreshExistingDevicePath(cd models.Device) {
+	d.lc.Debug("Refreshing existing device paths")
+	for _, fdPath := range cd.Protocols[UsbProtocol][Paths].([]interface{}) {


I am not sure if this is the proper approach. I think it might make more sense to find all of the /dev/video that this device uses and then compare them.

internal/driver/driver.go

ajcasagrande · 2023-06-21T23:36:55Z

internal/driver/driver.go

 	// Scan all paths to find the matching device.
 	// The file descriptor of video capture device can be /dev/video0 ~ 63
 	// https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/devices.txt#L1402-L1406
+	var init []interface{}


probably rename this function updateDevicePaths

ajcasagrande · 2023-06-21T23:40:29Z

internal/driver/driver.go

 	d.lc.Info("Discovery is triggered")
-
-	var devices []sdkModels.DiscoveredDevice
+	devices := make(map[string]sdkModels.DiscoveredDevice)


I am wondering for discovery if it makes more sense to take a more calculated approach, where we use v4l or something to discover the /dev/videos on a bus/device level rather than a plain scan of all video paths?

I wasn't sure if we wanted to look into a more complete refactor for discovery, but I will definitely look into it.

Signed-off-by: Darr, Christian <christian.darr@intel.com>

presatish

A couple of attributions are missing in attribution.txt, that file needs to be updated.

lenny-goodell · 2023-06-26T20:28:39Z

A couple of attributions are missing in attribution.txt, that file needs to be updated.

Are these adding new dependencies that are not used else where in EdgeX? If so please see https://wiki.edgexfoundry.org/display/FA/Vetting+Process+for+3rd+Party+Dependencies

lenny-goodell · 2023-06-26T20:29:56Z

@ChristianDarr-personal , your last commit is fail Semantic PR. Amend the commit to fix spelling.

Signed-off-by: Darr, Christian <christian.darr@intel.com>

presatish · 2023-06-26T20:34:23Z

A couple of attributions are missing in attribution.txt, that file needs to be updated.

Are these adding new dependencies that are not used else where in EdgeX? If so please see https://wiki.edgexfoundry.org/display/FA/Vetting+Process+for+3rd+Party+Dependencies

These are indirect dependencies, so do we still need to do what you have suggested?

lenny-goodell · 2023-06-26T20:42:44Z

These are indirect dependencies, so do we still need to do what you have suggested?

Nope, just on the direct dependencies.

Signed-off-by: Darr, Christian <christian.darr@intel.com>

lenny-goodell · 2023-06-27T00:38:57Z

Attribution.txt

+
+github.com/gabriel-vasile/mimetype (MIT) https://github.com/gabriel-vasile/mimetype
+https://github.com/gabriel-vasile/mimetype/blob/master/LICENSE
+
+github.com/klauspost/compress (Apache-2.0) https://github.com/klauspost/compress
+https://github.com/klauspost/compress/blob/master/LICENSE
+
+github.com/stretchr/objx (MIT) https://github.com/stretchr/objx
+https://github.com/stretchr/objx/blob/master/LICENSE
+
+google.golang.org/genproto/googleapis/rpc https://github.com/googleapis/go-genproto/blob/main/LICENSE
+https://github.com/googleapis/go-genproto/tree/main/googleapis/rpc


Where are these coming from? You didn't change the go.mod explicit section.

make test was still showing that I needed to include these for the attribution

Yes, this is because the go-mod indirect section changed when nothing changed in the explicit section

lenny-goodell · 2023-06-27T00:39:33Z

go.mod

 )

 require (
-	github.com/Microsoft/go-winio v0.6.0 // indirect


What triggered these updates?

When trying to compile, I ran go mod tidy after some build issues

I just ran go mod tidy and make build on main and nothing changed. Please rerun go mod tidy and see if anything changes. Something strange if it doesn't.

@ChristianDarr-personal Yes these changes seem to be not necessary and not sure how it happened. So you can just delete or comment out the indirect ones and run go mod tidy and that should restore the old dependencies back.

Signed-off-by: Darr, Christian <christian.darr@intel.com>

internal/driver/constants.go

presatish · 2023-06-27T14:31:36Z

internal/driver/device.go

@@ -8,13 +8,14 @@ package driver

 import (


Update copyright for this too.

internal/driver/driver.go

presatish · 2023-06-27T14:38:25Z

internal/driver/driver.go

+func getV4L2Output() ([]string, errors.EdgeX) {
+	cmd := exec.Command("v4l2-ctl", "--list-devices")
+	output, _ := cmd.Output()
+	// if err != nil {


There is no need for checking errors here?

This was something that I wanted to ask your advice on. So I was getting an error with this as the output:

Intel(R) RealSense(TM) Depth Ca (usb-0000:00:14.0-1.3): /dev/video1 /dev/video3 /dev/video5 /dev/video8 /dev/video9 /dev/video11 /dev/media0 /dev/media1 HD Pro Webcam C920 (usb-0000:00:14.0-1.4): /dev/video13 /dev/video14 /dev/media3 C270 HD WEBCAM (usb-0000:00:14.0-2.1): /dev/video6 /dev/video7 /dev/media2 Cannot open device /dev/video0, exiting.

This outputs the necessary information, but using the Output() function, it returns an error, and I can't access the info. So I elected to ignore that error for now, but I was wondering if you had any thoughts on how to handle this

internal/driver/driver.go

internal/driver/constants.go

Signed-off-by: preethi-satishcandra <preethi.satishchandra@intel.com>

Signed-off-by: Darr, Christian <christian.darr@intel.com>

Signed-off-by: preethi-satishcandra <preethi.satishchandra@intel.com>

Signed-off-by: Darr, Christian <christian.darr@intel.com>

ChristianDarr-personal added 7 commits June 21, 2023 13:56

feat: refactor path

60f360a

Signed-off-by: Darr, Christian <christian.darr@intel.com>

refactor: refactor discovery for multiple dev/videos

d9a0f4e

Signed-off-by: Darr, Christian <christian.darr@intel.com>

refactor: rename fields to support multiple paths

f174ede

Signed-off-by: Darr, Christian <christian.darr@intel.com>

test: add getPaths test

1b08cf3

Signed-off-by: Darr, Christian <christian.darr@intel.com>

test: add driver_test

2e28c22

Signed-off-by: Darr, Christian <christian.darr@intel.com>

test: add deviceCachedMap test

a73ab6c

Signed-off-by: Darr, Christian <christian.darr@intel.com>

refactor: update provision watcher

899c557

Signed-off-by: Darr, Christian <christian.darr@intel.com>

ajcasagrande reviewed Jun 21, 2023

View reviewed changes

ChristianDarr-personal added 3 commits June 22, 2023 15:37

refactor: change discovery method

77766c9

Signed-off-by: Darr, Christian <christian.darr@intel.com>

refactor: rediscovery

4171bb3

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: update variable names

1292bbc

Signed-off-by: Darr, Christian <christian.darr@intel.com>

ChristianDarr-personal changed the title ~~Extend discovery~~ refactor: extend discovery for realsense Jun 26, 2023

Merge branch 'main' into extend-discovery

bde17c6

ChristianDarr-personal marked this pull request as ready for review June 26, 2023 17:47

presatish changed the title ~~refactor: extend discovery for realsense~~ feat: extend discovery for realsense cameras Jun 26, 2023

presatish requested changes Jun 26, 2023

View reviewed changes

refactor: incorporate panic fixes

a5ce76a

Signed-off-by: Darr, Christian <christian.darr@intel.com>

ChristianDarr-personal force-pushed the extend-discovery branch from d047ebf to a5ce76a Compare June 26, 2023 20:34

ChristianDarr-personal added 2 commits June 26, 2023 15:52

build: add attributions

4f29463

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: add error for busInfo

8ca6cb3

Signed-off-by: Darr, Christian <christian.darr@intel.com>

lenny-goodell reviewed Jun 27, 2023

View reviewed changes

revert: remove go.mod changes

db3cde8

Signed-off-by: Darr, Christian <christian.darr@intel.com>

presatish requested changes Jun 27, 2023

View reviewed changes

refactor: cleaning up go mod and adding required attribution

8b9a288

Signed-off-by: preethi-satishcandra <preethi.satishchandra@intel.com>

ChristianDarr-personal and others added 11 commits June 27, 2023 11:23

feat: update comments

3e47c0f

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: refactor discovery for docker compatibilty

4367176

Signed-off-by: Darr, Christian <christian.darr@intel.com>

test: fix test type

3e6c44e

Signed-off-by: Darr, Christian <christian.darr@intel.com>

fix: avoid panic in rediscovery

22d2067

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: addressed more code review comments

473ff21

Signed-off-by: preethi-satishcandra <preethi.satishchandra@intel.com>

fix: fix panic during rediscovery

157e602

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: update path storage

b60069e

Signed-off-by: Darr, Christian <christian.darr@intel.com>

test: fix getPaths test

3307c6d

Signed-off-by: Darr, Christian <christian.darr@intel.com>

feat: update rediscovery method

a33123e

Signed-off-by: Darr, Christian <christian.darr@intel.com>

fix: close unused usb devices

d4a1bd4

Signed-off-by: Darr, Christian <christian.darr@intel.com>

fix: read commands

c54c24d

Signed-off-by: Darr, Christian <christian.darr@intel.com>

presatish mentioned this pull request Jun 30, 2023

feat: Extend discovery to support Intel real sense cameras #264

Merged

5 tasks

presatish closed this Jul 27, 2023

presatish deleted the extend-discovery branch August 16, 2023 18:18

Conversation

ChristianDarr-personal commented Jun 21, 2023 • edited by presatish Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

Uh oh!

codecov-commenter commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ajcasagrande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

presatish left a comment

Choose a reason for hiding this comment

Uh oh!

lenny-goodell commented Jun 26, 2023

Uh oh!

lenny-goodell commented Jun 26, 2023

Uh oh!

presatish commented Jun 26, 2023

Uh oh!

lenny-goodell commented Jun 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ChristianDarr-personal commented Jun 21, 2023 •

edited by presatish

Loading

codecov-commenter commented Jun 21, 2023 •

edited

Loading