feat: ability to disable RTSP server on startup via configuration by marcpfuller · Pull Request #268 · edgexfoundry/device-usb-camera

marcpfuller · 2023-08-08T22:51:07Z

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?)N/A
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?)
N/A

Testing Instructions

run make test
start up edgex using edgex-compose make run no-secty
build device-usb-camera make build
enable/ disable EnableRtspServer in canfiguration.yml
verify Rtsp server starts and does not start when enabled/disabled

New Dependency Instructions (If applicable)

codecov-commenter · 2023-08-08T23:05:35Z

Codecov Report

Merging #268 (e5325cd) into main (040a5a6) will decrease coverage by 0.12%.
The diff coverage is 0.00%.

❗ 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    #268      +/-   ##
========================================
- Coverage   4.21%   4.10%   -0.12%     
========================================
  Files          8       8              
  Lines       1067    1096      +29     
========================================
  Hits          45      45              
- Misses      1022    1051      +29

Files Changed	Coverage Δ
internal/driver/driver.go	`3.62% <0.00%> (-0.17%)`	⬇️

vyshali-chitikeshi · 2023-08-10T19:39:58Z

Validation looks good both in secure and non-secure mode deployments. Below are more details

When rtsp server disabled in configuration.yml file, streaming status, start streaming and stop
streaming APIs gives error "Device resource rtsp server is not enabled" as expected.
When rtsp server is enabled in configuration file, streaming status, start and stop streaming gives
no error and works as expected. Also noticed below msg in logs
"Starting rtsp authentication server on localhost:8000", This msg does not show up in logs when
rtsp is disabled.
Verified some general api's like camera status, camera info and camera status etc in both cases i.e.
rtsp server enabled/disabled and they work correctly
Negative test: Provided invalid value (something other than true/false) for rtsp server in configuration.yml file , noticed that it gave below error in logs, boot strapping exits before discovering cameras. I think this is expected.
level=ERROR ts=2023-08-10T19:15:40.54388004Z app=device-usb-camera source=init.go:77 msg="ProtocolDriver init failed: failed to parse EnableRtspServer config: strconv.ParseBool: parsing "abcd": invalid syntax"
Validated test cases from 1 to 4 both in secure and non secure modes, and results are same.

seanohair22

Tested, looks good! Just found one spelling error...

ajcasagrande · 2023-08-11T17:35:38Z

PR name should be updated. this is a feat. Also the current functionality is that it is enabled, so something like this:

feat: ability to disable RTSP server on startup via configuration

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

… server is disabled Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

ajcasagrande

Looks good! I have a few small suggestions.

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

…t os.stat message Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

ajcasagrande

LGTM

seanohair22

LGTM!

farshidtz

A late review. I'm not sure how this is suppose to work with docker and snaps, in which both have other defined ways of starting the RTSP server.

farshidtz · 2023-08-23T08:09:42Z

 	InputIndex                      = "InputIndex"
 	UrlRawQuery                     = "urlRawQuery"
+	EnableRtspServer                = "EnableRtspServer"
+	RtspServerCmd                   = "./rtsp-simple-server"


This should be made configurable. The executable path can vary depending on the deployment.

See the snap testing error, line 161:
Aug 15 00:10:47 fv-az570-769 edgex-device-usb-camera.device-usb-camera[5200]: level=ERROR ts=2023-08-15T00:10:47.372662735Z app=device-usb-camera source=init.go:77 msg="ProtocolDriver init failed: ./rtsp-simple-server file cannot be found: stat ./rtsp-simple-server: no such file or directory"
For the snap, it needs to be $SNAP/bin/rtsp-simple-server -> /snap/edgex-device-usb-camera/current/bin/rtsp-simple-server

marcpfuller force-pushed the enable_rtsp branch from cd7032c to a3527cb Compare August 8, 2023 22:52

seanohair22 suggested changes Aug 10, 2023

View reviewed changes

Comment thread internal/driver/driver.go Outdated

ajcasagrande self-requested a review August 11, 2023 17:36

marcpfuller requested a review from seanohair22 August 11, 2023 17:40

marcpfuller changed the title ~~fix: Enable Rtsp server at startup~~ feat: ability to disable RTSP server on startup via configuration Aug 11, 2023

Marc-Philippe Fuller added 3 commits August 11, 2023 10:41

feat: ability to disable RTSP server on startup via configuration

9491028

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

feat: Return error for streamuri and video streaming status when rtsp…

b307d1d

… server is disabled Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

fix: resolve spelling error

e5325cd

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

marcpfuller force-pushed the enable_rtsp branch from 1b9affd to e5325cd Compare August 11, 2023 17:42

ajcasagrande requested changes Aug 11, 2023

View reviewed changes

Comment thread internal/driver/driver.go Outdated

Comment thread internal/driver/driver.go Outdated

Comment thread internal/driver/driver.go Outdated

Comment thread internal/driver/driver.go Outdated

Comment thread internal/driver/driver.go Outdated

fix: Resolved all comments mentioned in PR by Anthony

38d3982

Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

marcpfuller requested a review from ajcasagrande August 11, 2023 22:14

ajcasagrande requested changes Aug 14, 2023

View reviewed changes

Comment thread internal/driver/driver.go Outdated

Comment thread internal/driver/driver.go

fix: Removed wg.Done from StartRTSPCredentialServer and change defaul…

f833c80

…t os.stat message Signed-off-by: Marc-Philippe Fuller <marc-philippe.fuller@intel.com>

marcpfuller force-pushed the enable_rtsp branch from cd43ff7 to f833c80 Compare August 15, 2023 00:02

ajcasagrande approved these changes Aug 15, 2023

View reviewed changes

seanohair22 approved these changes Aug 15, 2023

View reviewed changes

ajcasagrande merged commit 01aee5a into edgexfoundry:main Aug 15, 2023

farshidtz reviewed Aug 16, 2023

View reviewed changes

Comment thread cmd/res/configuration.yaml

Comment thread internal/driver/driver.go

Comment thread internal/driver/driver.go

marcpfuller mentioned this pull request Aug 22, 2023

fix: support external RTSP servers using RtspServerMode #270

Merged

5 tasks

farshidtz reviewed Aug 23, 2023

View reviewed changes

farshidtz mentioned this pull request Oct 13, 2023

build(deps): Bump github.com/edgexfoundry/device-sdk-go/v3 from 3.1.0-dev.33 to 3.1.0-dev.34 #301

Merged

farshidtz mentioned this pull request Oct 24, 2023

feat!: Remove snap packaging #303

Merged

5 tasks

Conversation

marcpfuller commented Aug 8, 2023

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

vyshali-chitikeshi commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanohair22 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajcasagrande commented Aug 11, 2023

Uh oh!

ajcasagrande left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajcasagrande left a comment

Choose a reason for hiding this comment

Uh oh!

seanohair22 left a comment

Choose a reason for hiding this comment

Uh oh!

farshidtz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farshidtz Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Aug 8, 2023 •

edited

Loading

vyshali-chitikeshi commented Aug 10, 2023 •

edited

Loading

farshidtz Aug 23, 2023 •

edited

Loading