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

Convert register data to string based on data type. #21

Closed
wants to merge 4 commits into from

Conversation

zongke
Copy link

@zongke zongke commented Apr 15, 2021

In the current version, after modbus collects data, it is reported to the cloud without data conversion.

@kubeedge-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kevin-wangzefeng after the PR has been reviewed.
You can assign the PR to them by writing /assign @kevin-wangzefeng in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubeedge-bot
Copy link
Collaborator

Welcome @zongke! It looks like this is your first PR to kubeedge/mappers-go 🎉

@kubeedge-bot kubeedge-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 15, 2021
Signed-off-by: zongke <zongk@shangyuantech.com>
Copy link
Collaborator

@sailorvii sailorvii left a comment

Choose a reason for hiding this comment

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

Many thanks for implementing the data conversion.

Topic: fmt.Sprintf(common.TopicDataUpdate, dev.Instance.ID)}
collectCycle := time.Duration(dev.Instance.Datas.Properties[i].PVisitor.CollectCycle)
collectCycle := time.Duration(dev.Instance.Datas.Properties[i].PVisitor.CollectCycle * 1e6)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The unit is a microsecond.

Copy link
Author

Choose a reason for hiding this comment

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

Time.Duration unit is Nanosecond,so if collectCycle unit is microsecond,we must multi 1e6.

Copy link
Collaborator

@sailorvii sailorvii Apr 19, 2021

Choose a reason for hiding this comment

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

Sorry, I'm wrong with the unit and I checked the history, the unit of the collect cycle is nano-second.
As I talked with the original author before, the reason why use nano-second is that golang could only deal with second and nano-second in JSON.

klog.V(2).Infof("Update value: %s, topic: %s", sData, td.Topic)
}

func TransferBytes(isRegisterSwap bool, isSwap bool, value []byte) []byte {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to new a file to add these converting functions.
You could also split this function into two:
swapRegister & swapByte. And the details could refer to the original implement:
https://github.com/kubeedge/kubeedge/blob/dfcdab01d4538ebefc2284a1b82a407d649e8f94/mappers/modbus_mapper/src/common.js#L70

Copy link
Author

Choose a reason for hiding this comment

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

ok,I will take a look.

Copy link
Author

Choose a reason for hiding this comment

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

The following is my understanding, and my code can achieve it:
AB CD : isRegisterSwap:false isSwap:false
CD AB : isRegisterSwap:true isSwap:false
BA DC : isRegisterSwap:false isSwap:trur
DC BA : isRegisterSwap:true isSwap:true

But according to the code logic,when isRegister is true ,it will be DC BA!

I'm a little confused, can you explain these two parameters for me?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your implement is right.
However, it's more clear to split into two functions: swapRegister and swapByte.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, i will fix it!

switch len(value) {
case 1:
var tmp int8
err := binary.Read(byteBuff, binary.BigEndian, &tmp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not clear about the big-endian impact.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please skip this comment.

zongke added 2 commits April 20, 2021 13:03
Signed-off-by: zongke <zongk@shangyuantech.com>

# Conflicts:
#	pkg/modbus/device/twindata.go
Copy link
Collaborator

@sailorvii sailorvii left a comment

Choose a reason for hiding this comment

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

Many thanks for this change. Fix and ship.

@@ -0,0 +1,122 @@
/*
Copyright 2020 The KubeEdge Authors.
Copy link
Collaborator

Choose a reason for hiding this comment

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

2020 -> 2021

"strconv"
)

func SwitchRegister(value []byte) []byte {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Annotation for public function.

@zongke zongke closed this Apr 20, 2021
@zongke
Copy link
Author

zongke commented Apr 20, 2021

new pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants