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

feat: supply string type #262

Merged
merged 9 commits into from
Aug 3, 2021
Merged

feat: supply string type #262

merged 9 commits into from
Aug 3, 2021

Conversation

hongda3141
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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

What is the current behavior?

Issue Number:

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

New Imports

  • Yes
  • No

Specific Instructions

Other information

Signed-off-by: vm_ubuntu20.04 <[email protected]>
@hongda3141
Copy link
Contributor Author

change log:

  1. modify variable "stringLength" to "wordLength"
  2. change bit length of string from 1 to 16
  3. change swith-case to if-else block

@hongda3141
Copy link
Contributor Author

@weichou1229
PTAL again ,thanks

Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

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

internal/driver/deviceclient.go Outdated Show resolved Hide resolved
internal/driver/deviceclient.go Outdated Show resolved Hide resolved
internal/driver/constant.go Outdated Show resolved Hide resolved
@hongda3141
Copy link
Contributor Author

@weichou1229
PTAL again ,thanks a lot for the advice

Copy link
Member

@weichou1229 weichou1229 left a comment

Choose a reason for hiding this comment

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

No problem.
Since we change the data model in V2, please verify the GET and PUT command works with edgex-go master branch.

cmd/res/devices/modbus.test.devices.toml Outdated Show resolved Hide resolved
cmd/res/profiles/modbus.test.device.string.profile.yml Outdated Show resolved Hide resolved
cmd/res/profiles/modbus.test.device.string.profile.yml Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2021

Codecov Report

Merging #262 (98f7e93) into main (5a20fcc) will decrease coverage by 1.08%.
The diff coverage is 29.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   42.25%   41.16%   -1.09%     
==========================================
  Files           6        6              
  Lines         542      566      +24     
==========================================
+ Hits          229      233       +4     
- Misses        275      293      +18     
- Partials       38       40       +2     
Impacted Files Coverage Δ
internal/driver/deviceclient.go 67.97% <29.03%> (-8.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 5a20fcc...98f7e93. Read the comment docs.

@hongda3141
Copy link
Contributor Author

@weichou1229
that`s right, profiles should update to v2 too,
PTAL again ,thanks

@weichou1229
Copy link
Member

weichou1229 commented Jun 16, 2021

@hongda3141
The Get and Put command works incorrectly.

  1. For the Get command, I thought the value should decode to a readable string.
    "value" : "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"
% curl http://localhost:59882/api/v2/device/name/Modbus-TCP-Read-String/StringB | json_pp
{
   "apiVersion" : "v2",
   "event" : {
      "apiVersion" : "v2",
      "deviceName" : "Modbus-TCP-Read-String",
      "id" : "669f5d9f-2004-47ad-9533-e15ff452304d",
      "origin" : 1623834796385661000,
      "profileName" : "Test.Device.Modbus.String.Profile",
      "readings" : [
         {
            "binaryValue" : null,
            "deviceName" : "Modbus-TCP-Read-String",
            "id" : "3fb25504-d6da-4525-8143-3a55f84299ad",
            "mediaType" : "",
            "origin" : 1623834796385624000,
            "profileName" : "Test.Device.Modbus.String.Profile",
            "resourceName" : "StringB",
            "value" : "\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000",
            "valueType" : "String"
         }
      ],
      "sourceName" : "StringB"
   },
   "statusCode" : 200
}
  1. For the Put command, I got the transform error.
% curl http://localhost:59882/api/v2/device/name/Modbus-TCP-Read-String/StringB -X PUT -H "Content-Type: application/json" -d '{
    "StringB":"test-string"
  }' | json_pp
{
   "apiVersion" : "v2",
   "message" : "request failed, status code: 500, err: {\"apiVersion\":\"v2\",\"message\":\"error writing DeviceResourece StringB for Modbus-TCP-Read-String -\\u003e transform command value failed, err: failed to transform test-string to []byte\",\"statusCode\":500}",
   "statusCode" : 500
}

@hongda3141
Copy link
Contributor Author

hongda3141 commented Jun 21, 2021

thanks very much,i`ll fix that

@hongda3141
Copy link
Contributor Author

No problem.
Since we change the data model in V2, please verify the GET and PUT command works with edgex-go master branch.

@weichou1229 hi,i found there is no edgex-lanch.sh in branch master, could you tell me which branch did you used for edgex-go? i always used and tested in fuji, but i think maybe it is too old for v2 version.

@weichou1229
Copy link
Member

@hongda3141

i found there is no edgex-lanch.sh in branch master

It was removed in master branch

could you tell me which branch did you used for edgex-go?

master branch

You can use the following script before community add it back.

#!/bin/bash

DIR=$PWD
CMD=../cmd

# Kill all edgex-* stuff
function cleanup {
	pkill edgex
}

# disable secret-store integration
export EDGEX_SECURITY_SECRET_STORE=false

###
# Core Command
###
cd $CMD/core-command
# Add `edgex-` prefix on start, so we can find the process family
exec -a edgex-core-command ./core-command &
cd $DIR

###
# Core Data
###
cd $CMD/core-data
exec -a edgex-core-data ./core-data &
cd $DIR

###
# Core Metadata
###
cd $CMD/core-metadata
exec -a edgex-core-metadata ./core-metadata &
cd $DIR

trap cleanup EXIT

while : ; do sleep 1 ; done

@hongda3141
Copy link
Contributor Author

thanks a lot

Signed-off-by: vm_ubuntu20.04 <[email protected]>
@hongda3141
Copy link
Contributor Author

@weichou1229
i fixed the PUT and GET command error,
PTAL thanks

internal/driver/deviceclient.go Outdated Show resolved Hide resolved
cmd/res/devices/modbus.test.devices.toml Outdated Show resolved Hide resolved
@hongda3141
Copy link
Contributor Author

@weichou1229
i update the codes with using build-in method for trimming the zero value,
PTAL thanks

Copy link
Member

@weichou1229 weichou1229 left a comment

Choose a reason for hiding this comment

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

LGTM

@weichou1229 weichou1229 requested a review from cloudxxx8 June 29, 2021 01:13
@cloudxxx8 cloudxxx8 added the hold Intended for PRs we want to flag for ongoing review label Jun 29, 2021
@cloudxxx8
Copy link
Member

hold until 2.0.0 release
this is targeting to 2.0.1

Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

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

LGTM

@cloudxxx8 cloudxxx8 removed the hold Intended for PRs we want to flag for ongoing review label Aug 3, 2021
@cloudxxx8 cloudxxx8 merged commit c8e345b into edgexfoundry:main Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants