bluetooth mapper #13

ttlv · 2021-04-08T04:57:20Z

it's the refactor version of bluetooth mapper

kubeedge-bot · 2021-04-08T04:57:29Z

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

fisherxu

Do we need to delete the old version of bluetooth mapper?

sailorvii

Many thanks for making this change. It's OK overall but some minor comments.

sailorvii · 2021-04-08T07:08:42Z

pkg/ble/device/twindata.go

+			// If this Characteristic suports notifications and there's a CCCD
+			// Then subscribe to it
+			if (c.Property&ble.CharNotify) != 0 && c.CCCD != nil {
+				if err := td.BluetoothClient.Client.Subscribe(c, false, td.notificationHandler()); err != nil {


Not clear why there's a subscription. This function will be called periodically. If the period is small, then there will be lots of subscribing and publishing.

well,because bluetooth support two method to fetch data,one is read and another is notify,thers is no clearly defination in crd,so I don't know it's read or notify.

sailorvii · 2021-04-08T07:08:49Z

pkg/ble/device/twindata.go

+			c := u.(*ble.Characteristic)
+			// read data actively
+			if (c.Property & ble.CharRead) != 0 {
+				b, err := td.BluetoothClient.Client.ReadCharacteristic(c)


It's better to pack it to a read function that aligned with write.

/lgtm, it had been fixed

sailorvii · 2021-04-08T07:08:52Z

pkg/ble/device/twindata.go

+
+// Run timer function.
+func (td *TwinData) Run() {
+	uuid := ble.MustParse(td.BluetoothVisitorConfig.CharacteristicUUID)


It's better to move the repeated finding out of the function "Run" to the function "initTwin /Data".

/lgtm, it had been fixed

sailorvii · 2021-04-08T07:09:13Z

pkg/ble/device/twindata.go

+}
+
+// ConvertReadData is the function responsible to convert the data read from the device into meaningful data
+// If currently logic of Convert data is not suitbale for your deive,you can change ConvertReadData function manually


space after ",", and "." at the end.

/lgtm, it had been fixed

ttlv · 2021-04-08T08:54:25Z

Do we need to delete the old version of bluetooth mapper?

Yeap,the code of old version of bluetooth mapper is diffiult to read and what's more it can not run,so I suggest that it's better to delete.

Many thanks for making this change. It's OK overall but some minor comments.

thanks for your review

sailorvii

Thanks for the refinement.

I'm confused about the ble & bluetooth. I thought if the mapper is for Bluetooth low energy device, then unify the function & variable name to "ble". Otherwise, unify them to bl/bluetooth.

sailorvii · 2021-04-12T01:15:03Z

pkg/ble/Makefile

+SHELL = /bin/bash
+
+build:
+	@GOOS=linux CGO_ENABLED=0 GOARCH=arm go build -o ble-go


The arm platform should be notified to the user.

/lgtm, it had been fixed

sailorvii · 2021-04-12T01:16:51Z

pkg/ble/configmap/type.go

+
+package configmap
+
+// ModbusVisitorConfig is the modbus register configuration.


Refine the name.

/lgtm, it had been fixed

sailorvii · 2021-04-12T01:17:33Z

pkg/ble/configmap/type.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2020 The KubeEdge Authors.


Refine the year. Same about other files.

/lgtm, it had been fixed

sailorvii · 2021-04-12T01:18:38Z

pkg/ble/device/device.go

+	}
+}
+
+// initModbus initialize modbus client


Remove all "modbus".

/lgtm, it had been fixed

sailorvii · 2021-04-12T01:22:07Z

pkg/ble/device/device.go

+			}
+			timer := mappercommon.Timer{Function: twinData.Run, Duration: collectCycle, Times: 0}
+			// If this Characteristic suports notifications and there's a CCCD
+			// Then subscribe to it, the notifications operation is different from reading operation, notifications will keep looping when connected


Correct the capital and small letter.
imer->timer

/lgtm, it had been fixed

sailorvii · 2021-04-12T01:24:38Z

pkg/ble/driver/client.go

@@ -0,0 +1,66 @@
+package driver


/lgtm, it had been fixed

sailorvii · 2021-04-12T01:24:53Z

pkg/ble/driver/client.go

+	Addr string
+}
+
+// OPCUAClient is the client structure.


BluetoothClient

/lgtm, it had been fixed

ttlv · 2021-04-12T04:20:29Z

Thanks for the refinement.

I'm confused about the ble & bluetooth. I thought if the mapper is for Bluetooth low energy device, then unify the function & variable name to "ble". Otherwise, unify them to bl/bluetooth.

In fact, Bluetooth(BT) and BLE are different and the old mapper for bluetooth just for ble.ble includes GATT，ATT HCI and so on, BLE is used mostly in IOT devices because of low energy.so we should name it as BLE and not as bluetooth。WDYT @fisherxu @sailorvii

sailorvii

Address the remaining issue and pass. /approve.
BTW, please merge the commits.

sailorvii · 2021-04-13T02:09:52Z

pkg/ble/configmap/type.go

+
+package configmap
+
+// BLEVisitorConfig is the BLE register configuration.


Refine the name.

sailorvii · 2021-04-13T02:11:06Z

pkg/ble/device/device.go

+	}
+}
+
+// initBLE initialize ble client


Refine the function name either comment or function.

kubeedge-bot · 2021-04-13T02:12:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sailorvii

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sailorvii]

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

fisherxu · 2021-04-13T02:41:46Z

@ttlv @sailorvii Thanks for the great work! And it's best to add /lgtm and /approve label after all comments are addressed. @sailorvii :)

ttlv added 6 commits April 5, 2021 14:55

feat/ble: init code

124bb5c

feat/config: add config yaml

8423c77

feat/global: rename

2f0b729

fix/device: fix data convert

3b8a1f0

feat/twin: suuport read data actively

65497c5

feat/test: add test case for comfigmap parse

740cd81

kubeedge-bot requested review from kevin-wangzefeng and sailorvii April 8, 2021 04:57

kubeedge-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 8, 2021

feat/deploy: add dockerfile and makefile and deployment yaml

f4242e8

kubeedge-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 8, 2021

feat/dockerfile: fix dockerfile

28bcbf8

ttlv mentioned this pull request Apr 8, 2021

refactor bluetooth mapper kubeedge/kubeedge#2249

Closed

fisherxu reviewed Apr 8, 2021

View reviewed changes

sailorvii reviewed Apr 8, 2021

View reviewed changes

ttlv added 4 commits April 8, 2021 17:02

fix/device: fix twin

b3a5dc0

fix/device: fix initTwin and initData

d362958

fix/device: remove find logic out of run function

f3b2094

fix/twin: move notify out of twin loop

d569965

sailorvii reviewed Apr 12, 2021

View reviewed changes

fix/global: add copyright

7b1e4e2

sailorvii approved these changes Apr 13, 2021

View reviewed changes

kubeedge-bot assigned sailorvii Apr 13, 2021

kubeedge-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2021

kubeedge-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2021

kubeedge-bot merged commit 6307a3d into kubeedge:main Apr 13, 2021

fisherxu mentioned this pull request Apr 14, 2021

Remove mappers from kubeedge/kubeedge repo kubeedge/kubeedge#2774

Merged

ttlv mentioned this pull request Apr 15, 2021

REQUEST: New membership for ttlv kubeedge/community#48

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bluetooth mapper #13

bluetooth mapper #13

ttlv commented Apr 8, 2021

kubeedge-bot commented Apr 8, 2021

fisherxu left a comment

sailorvii left a comment

sailorvii Apr 8, 2021

ttlv Apr 8, 2021

sailorvii Apr 8, 2021

ttlv Apr 8, 2021

sailorvii Apr 8, 2021

ttlv Apr 8, 2021

sailorvii Apr 8, 2021

ttlv Apr 8, 2021

ttlv commented Apr 8, 2021 •

edited

Loading

sailorvii left a comment

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

sailorvii Apr 12, 2021

ttlv Apr 12, 2021

ttlv commented Apr 12, 2021 •

edited

Loading

sailorvii left a comment

sailorvii Apr 13, 2021

sailorvii Apr 13, 2021

kubeedge-bot commented Apr 13, 2021

fisherxu commented Apr 13, 2021


		package configmap

		// ModbusVisitorConfig is the modbus register configuration.


		package configmap

		// BLEVisitorConfig is the BLE register configuration.

bluetooth mapper #13

bluetooth mapper #13

Conversation

ttlv commented Apr 8, 2021

kubeedge-bot commented Apr 8, 2021

fisherxu left a comment

Choose a reason for hiding this comment

sailorvii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttlv commented Apr 8, 2021 • edited Loading

sailorvii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttlv commented Apr 12, 2021 • edited Loading

sailorvii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubeedge-bot commented Apr 13, 2021

fisherxu commented Apr 13, 2021

ttlv commented Apr 8, 2021 •

edited

Loading

ttlv commented Apr 12, 2021 •

edited

Loading