Implemented optional on screen usage of Surface Dial, reworked some o… #169

OmegaRogue · 2019-03-17T19:29:07Z

…ther stuff

hansmbakker

Hi,
thank you for your PR! I'm requesting some changes.

Please review my comments in the code
Please update the sample so that others can see how to use your new functions.

Also, did you test this code yet?

hansmbakker · 2019-03-17T21:18:45Z

src/HID-APIs/SurfaceDialAPI.h

@@ -49,17 +49,29 @@ class SurfaceDialAPI
  inline void end(void);
  inline void click(void);
  inline void rotate(int16_t rotation);
+	inline void position(int8_t x, int8_t y);
+	inline void posrot(int16_t rotation, int8_t x, int8_t y);


I would like a more readable name here

hansmbakker · 2019-03-17T21:20:03Z

src/HID-APIs/SurfaceDialAPI.h

@@ -49,17 +49,29 @@ class SurfaceDialAPI
  inline void end(void);
  inline void click(void);
  inline void rotate(int16_t rotation);
+	inline void position(int8_t x, int8_t y);


please review your indentation settings. now it looks like it is wrongly indented.

hansmbakker · 2019-03-17T21:23:57Z

src/HID-APIs/SurfaceDialAPI.hpp

+	report.rotation = 0;
+	if(_onScreen)
+	{
+		report.xAxis = _xAxis;


validation of a correct value is not done yet

how should the values be validated?

ah, I see in the docs that you can specify values out of range to remove the device from the screen

Which Docs say that you can specify values out of the range to remove the device from the screen?

hansmbakker · 2019-03-17T21:25:55Z

src/HID-APIs/SurfaceDialAPI.hpp

+	update();
+}
+
+void SurfaceDialAPI::update()


Not very intuitive what the difference between posrot and update is. It turns out the difference is in the rotation variable.

hansmbakker · 2019-03-17T21:26:38Z

src/HID-APIs/SurfaceDialAPI.hpp

+	posrot(0, x, y);
+}
+
+void SurfaceDialAPI::posrot(int16_t rotation, int8_t x, int8_t y)


Please change this name into something more readable if you intend to keep this function

hansmbakker · 2019-03-17T21:27:57Z

src/MultiReport/SurfaceDial.cpp

+    0x46, 0xb0, 0x36, //     PHYSICAL_MAXIMUM (14000)
+    0x81, 0x02,       //     INPUT (Data,Var,Abs)
+    0x05, 0x0d,       //     USAGE_PAGE (Digitizers)
+    0x09, 0x48,       //     USAGE (Width)


Windows expects a value for the width if you want to use the position reporting.

but the width is constant, minimum value is 3000, maximum value is 3000

OmegaRogue · 2019-03-18T08:48:30Z

I haven't tested it yet, and ill make changes to the code regarding your comments and update the sample, also ive added a long click/ short click detection to a modified version of the sample i use myself

OmegaRogue · 2019-03-18T19:10:48Z

ok now everything should be fixed, the arduino sketch compiles, but it doesnt put it on screen

OmegaRogue · 2019-03-18T19:42:01Z

and i have no idea why

hansmbakker · 2019-03-20T17:10:37Z

Does it work with my code? So what is currently in master? (without position)

.idea/codeStyles/codeStyleConfig.xml

.idea/deployment.xml

.idea/vcs.xml

hansmbakker · 2019-03-20T17:15:21Z

examples/SurfaceDial/SurfaceDial.ino

+unsigned long keyPrevMillis = 0;
+const unsigned long keySampleIntervalMs = 25;
+byte longKeyPressCountMax = 840;    // 80 * 25 = 2000 ms
+byte longKeyPressCount = 0;


why does this work with a count rather than with millis directly?

idk i found the code online and adapted it for this purpose

hansmbakker · 2019-03-20T17:24:06Z

src/HID-APIs/SurfaceDialAPI.h

@@ -32,12 +32,15 @@ typedef union{
 	uint8_t whole8[0];
 	uint16_t whole16[0];
 	uint32_t whole32[0];
+	uint64_t whole64[0];


I believe these wholeX variables are actually not needed, see #147
If you want you can remove them after testing.

@hansmbakker Would you mind checking the project where those variables are used, and remove them entirely from the project? Maybe the uin8_t version could be kept with the name "raw", but a proper length value.

@NicoHood unfortunately I'm a bit busy on other things to pick this up quickly; also it is not immediately clear to me what you mean with

but a proper length value

@NicoHood or @OmegaRogue is that something you could pick up?

@NicoHood what would the length be?

hansmbakker · 2019-03-20T17:39:33Z

src/HID-APIs/SurfaceDialAPI.h

@@ -49,17 +52,30 @@ class SurfaceDialAPI
  inline void end(void);
  inline void click(void);
  inline void rotate(int16_t rotation);
+  inline void position(int16_t x, int16_t y);


W.r.t. the data type - shouldn't the X and Y coordinates be unsigned?

but isnt 0, 0 the screen center?

hansmbakker · 2019-03-20T17:50:57Z

src/HID-APIs/SurfaceDialAPI.hpp

+	report.rotation = 0;
+	if(_onScreen)
+	{
+		report.xAxis = _xAxis;


ah, I see in the docs that you can specify values out of range to remove the device from the screen

hansmbakker · 2019-03-20T17:51:12Z

src/MultiReport/SurfaceDial.cpp

+    0x46, 0xb0, 0x36, //     PHYSICAL_MAXIMUM (14000)
+    0x81, 0x02,       //     INPUT (Data,Var,Abs)
+    0x05, 0x0d,       //     USAGE_PAGE (Digitizers)
+    0x09, 0x48,       //     USAGE (Width)


NicoHood · 2019-03-31T06:37:35Z

I just reviewed the comments from @hansmbakker, all his request are valid. Please update accordingly.

Pulling the new Commits from NicoHood/HID to this fork

Sync with head repo

Implemented optional on screen usage of Surface Dial, reworked some o…

158e0bd

…ther stuff

OmegaRogue mentioned this pull request Mar 17, 2019

what is the position data in the surface dial report for? #167

Open

hansmbakker suggested changes Mar 17, 2019

View reviewed changes

OmegaRogue and others added 7 commits March 18, 2019 17:38

j

fd499d0

changes

736d5a4

Updated Example

d9507f9

Updated Example

951f030

fixed OnScreen functionality

1fe6764

fixed OnScreen functionality

0fad9ff

fixed OnScreen functionality

c57f3bc

hansmbakker suggested changes Mar 20, 2019

View reviewed changes

OmegaRogue added 5 commits April 24, 2019 16:15

Merge pull request #1 from NicoHood/master

fd29511

Pulling the new Commits from NicoHood/HID to this fork

Delete vcs.xml

cad668f

Delete deployment.xml

fab7ec9

Delete codeStyleConfig.xml

df80f3e

Merge pull request #2 from NicoHood/master

935e984

Sync with head repo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented optional on screen usage of Surface Dial, reworked some o… #169

Implemented optional on screen usage of Surface Dial, reworked some o… #169

OmegaRogue commented Mar 17, 2019

hansmbakker left a comment •

edited

Loading

hansmbakker Mar 17, 2019

hansmbakker Mar 17, 2019

hansmbakker Mar 17, 2019

OmegaRogue Mar 18, 2019

hansmbakker Mar 20, 2019

OmegaRogue Apr 24, 2019

hansmbakker Mar 17, 2019

hansmbakker Mar 17, 2019

hansmbakker Mar 17, 2019

OmegaRogue Mar 18, 2019

hansmbakker Mar 20, 2019

OmegaRogue commented Mar 18, 2019

OmegaRogue commented Mar 18, 2019

OmegaRogue commented Mar 18, 2019

hansmbakker commented Mar 20, 2019

hansmbakker Mar 20, 2019

OmegaRogue Mar 20, 2019

hansmbakker Mar 20, 2019

NicoHood Mar 31, 2019

hansmbakker Apr 1, 2019

OmegaRogue Apr 24, 2019

hansmbakker Mar 20, 2019

OmegaRogue Apr 24, 2019

hansmbakker Mar 20, 2019

hansmbakker Mar 20, 2019

NicoHood commented Mar 31, 2019

Implemented optional on screen usage of Surface Dial, reworked some o… #169

Are you sure you want to change the base?

Implemented optional on screen usage of Surface Dial, reworked some o… #169

Conversation

OmegaRogue commented Mar 17, 2019

hansmbakker left a comment • edited Loading

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

OmegaRogue commented Mar 18, 2019

OmegaRogue commented Mar 18, 2019

OmegaRogue commented Mar 18, 2019

hansmbakker commented Mar 20, 2019

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

NicoHood commented Mar 31, 2019

hansmbakker left a comment •

edited

Loading