Add support for dimmer curves per E1.37-1 by kecramer · Pull Request #1476 · OpenLightingProject/ola

kecramer · 2018-08-20T20:50:40Z

Tried to imitate the style and conventions of similar endpoints as best I could.

_HandleU16Response was used in the first iteration of this endpoint, but was no longer necessary when I moved the response to a struct. I left the method there as it seems like it could be useful in the future? It can easily be removed though.

peternewman

Thanks for this @kecramer ! Some comments, mostly fairly minor.

The blue sky ideal is to rewrite this whole bit to do something dynamic which will hence support all PIDs (including manufacturer ones) by generating JSON schema and a dynamic UI (although it will still need some smarts for linked PIDs like these). It also needs lots of coding, so feel free to keep hacking in quick fix copy/pastes like these where appropriate. The current web UI also unfortunately doesn't support sub-devices, which a lot of dimmers may represent themselves as.

peternewman · 2018-08-20T22:27:06Z


+/*
+ * Fetch the dimmer curve
+ * @param uid the UID to fetch the DNS domain name for


Copy/Paste typo

peternewman · 2018-08-20T22:28:03Z

+
+/*
+ * Fetch the dimmer curve description (name)
+ * @param uid the UID to fetch the DNS domain name for


peternewman · 2018-08-20T22:30:48Z

 }

+/*
+ * Fetch the dimmer curve


In theory these, and all the others already in the file should be @brief, but given we haven't enabled any of this as Doxygen for some reason, it doesn't really matter right now. But if you felt like fixing them, that would be awesome 👍

Is this only for comment blocks with @params after them, or all comment blocks preceding a function call? I can take care of them in one go here.

Generally if it's only one line we want to tag it as brief because then Doxygen can do some magic and show it in a few more places:
https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdbrief

It would be all Doxygen style ones, so if they've already got @param or @return or whatever in there too.

peternewman · 2018-08-20T22:32:48Z

+
+/*
+ * Set the dimmer curve
+ * @param uid the UID to set the DNS domain name for


peternewman · 2018-08-20T22:33:43Z

+ * @param error a pointer to a string which it set if an error occurs
+ * @return true if the request is sent correctly, false otherwise
+ */
+bool RDMAPI::SetCurve(


Order wise, we probably want Get then Set for the same PID, so GetCurve, SetCurve, GetCurveDescription.

peternewman · 2018-08-20T23:09:41Z

+    if (i <= info->curves.size()) {
+      ostringstream str;
+      str << info->curves[i - 1].second << " (" <<
+        info->curves[i - 1].first << ")";


Can you move the << on the line above to this line and align them.

You may want to not print the () if the description is empty; I'm not sure what we do elsewhere regarding this.

peternewman · 2018-08-20T23:11:04Z

+
+  section.AddItem(item);
+  section.AddItem(new StringItem("Available Curves",
+    std::to_string(info->total)));


Can you align this with the second ( above please.

peternewman · 2018-08-20T23:12:31Z

+      unsigned int active;
+      unsigned int next;
+      unsigned int total;
+      std::vector<std::pair<uint32_t, std::string> > curves;


peternewman · 2018-08-20T23:13:09Z

    static const char RESET_DEVICE_SECTION[];
    static const char SENSOR_SECTION[];
    static const char TILT_INVERT_SECTION[];
+    static const char CURVE_SECTION[];


Again ordering.

peternewman · 2018-08-20T23:13:15Z

    static const char PROXIED_DEVICES_SECTION_NAME[];
    static const char RESET_DEVICE_SECTION_NAME[];
    static const char TILT_INVERT_SECTION_NAME[];
+    static const char CURVE_SECTION_NAME[];


peternewman · 2018-08-21T14:39:58Z

+
+  section.AddItem(item);
+  section.AddItem(new StringItem("Available Curves",
+                                 std::to_string(info->total)));


This still needs converting to our IntToString function.

peternewman · 2018-08-21T22:50:27Z

  }

-  info->curves.push_back(pair<uint32_t, string>(curve, description));
+  info->curve_descriptions.push_back(description);


So now you've made this change, the curve parameter should now be OLA_UNUSED again!

peternewman · 2018-08-21T22:53:05Z

  for (unsigned int i = 1; i <= info->total; i++) {
-    if (i <= info->curves.size()) {
+    if (i <= info->curve_descriptions.size() &&
+        info->curve_descriptions[i - 1].length() > 0) {


You can use !.empty() for the string.

peternewman · 2018-08-22T16:02:06Z

  for (unsigned int i = 1; i <= info->total; i++) {
    if (i <= info->curve_descriptions.size() &&
-        info->curve_descriptions[i - 1].length() > 0) {
+        info->curve_descriptions[i - 1].empty() == false) {


You can just use:
!info->curve_descriptions[i - 1].empty()

So the total bit is:

if (i <= info->curve_descriptions.size() && !info->curve_descriptions[i - 1].empty()) {

Which should be more readable.

Sure thing, happy to change that. I've found using == false makes it more explicit in my personal coding.

Yeah I think we generally go for ! rather than == false in the rest of the codebase. Personally the latter makes me do a bit of a double-take.

peternewman

LGTM, just one more minor grammar nit.

peternewman · 2018-08-22T17:03:38Z

+ * @param uid the UID to fetch the dimmer curve information for
+ * @param sub_device the sub device to use
+ * @param callback the callback to invoke when this request completes
+ * @param error a pointer to a string which it set if an error occurs


I don't know if this typo exists throughout, but it should be "which IS set", can you fix it wherever necessary please.

Looks like it was present across all the functions. Changed all occurrences in RDMAPI.cpp

Kevin Cramer added 8 commits August 16, 2018 21:42

Add RDM handlers for receiving dimmer curve data

adfee68

fix linter error

f739858

Add handlers to set the curve

3304907

Add comment to explain bitwise operations

2f05039

Make function docs more complete.

fdeb3e3

Parse CURVE response the ola way

3f124d5

Separate http response from rdm response. Setup for getting descriptions

a54c21c

Add handlers to grab dimmer curve descriptions and display in olad-www

72fddc9

peternewman requested changes Aug 20, 2018

View reviewed changes

Kevin Cramer added 3 commits August 21, 2018 06:56

Fix comment issues

122cba0

Code ordering and alignment

b2862a0

Code ordering and alignment

1397366

peternewman requested changes Aug 21, 2018

View reviewed changes

Kevin Cramer added 2 commits August 21, 2018 07:46

Only store descriptions vs curve #/description pair.

d95ebef

Clean up response if device doesn't handle curve_description well.

486fa29

peternewman reviewed Aug 21, 2018

View reviewed changes

code review changes

ed092b0

peternewman reviewed Aug 22, 2018

View reviewed changes

style change

07ce28b

peternewman added this to the 0.11.0 milestone Aug 22, 2018

peternewman approved these changes Aug 22, 2018

View reviewed changes

Kevin Cramer and others added 2 commits August 22, 2018 10:27

fix grammar in comments

0958b6e

Merge branch 'master' into add-curve

4f26d47

peternewman assigned nomis52 Aug 22, 2018

Add some more codespellignore

8dd2c2d

peternewman assigned peternewman and unassigned nomis52 Sep 6, 2018

peternewman merged commit fa72097 into OpenLightingProject:master Sep 8, 2018

peternewman mentioned this pull request Jul 12, 2026

Add 0.11 news #2060

Draft

Uh oh!

Conversation

kecramer commented Aug 20, 2018

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman Aug 21, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peternewman Aug 21, 2018 •

edited

Loading