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

Introduce constant types #2007

Merged
merged 28 commits into from
Sep 4, 2020
Merged

Introduce constant types #2007

merged 28 commits into from
Sep 4, 2020

Conversation

evgenykochetkov
Copy link
Contributor

Values of constant types are known at compile time, and can't be changed during the runtime.
This allows for some compile-time checks and optimisations.

The value of a constant inputs are accessible as constant_input_PINNAME, as well as through regular getValue function.

The value of a constant output has to be declared like static const TypeOfPINNAME constant_input_PINNAME = value;.

Types of inputs and outputs are accessible as TypeOfPINNAME instead of ValueType<output_PINNAME>::T, and are available before declaring State, allowing some truly generic implementations.

Custom types that have constant parameters also some receive special treatment. Types of such outputs have to be declared manually. For example, typedef TypeOfIN TypeOfOUT; or typedef some__node<new_constant_value>::Type TypeOfOUT;. The typedef TypeOfIN TypeOfOUT; case is automated if the output pin is named like input pin, but with an ' at the end, like DEV and DEV'.

@evgenykochetkov evgenykochetkov self-assigned this Jul 15, 2020
@@ -56,12 +56,16 @@ const TPatchOutput = Model('TPatchOutput', {
value: DataValue,
isDirtyable: $.Boolean,
isDirtyOnBoot: $.Boolean,
// isTemplatableCustomTypePin: $.Boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these lines be commented on?

workspace/__lib__/xod-dev/hc-sr04/ping/patch.cpp Outdated Show resolved Hide resolved
@nkrkv
Copy link
Member

nkrkv commented Jul 17, 2020

So, from what I see, almost all the code of a node implementation is implicitly enclosed in a struct:

namespace xod {
// ...
struct xod__common_hardware__text_lcd_16x2 {
  // ...
  
  void printLine(LiquidCrystal* lcd, uint8_t lineIndex, XString str) { ... }
  void evaluate(Context ctx) { ... }
};
}

This may lead to silly surprises: worse performance, no way to get a pointer to a seemingly zero-argument function, etc. I’m sure, that putting something inside a struct/class should be visible and explicit to a xoder.

So, how to acheive this? Some experiments are required but here’s what I think in rough detail. First, let’s see what a node implementation looks like now:

struct State { int xMemo; };
using Type = uint32_t;
/* Bla bla I’m a top comment */

// Handlebars hack to render generated boilerplate
// *and* to split the source in two parts
{{ GENERATED_CODE }}

/* Bla bla I’m a bottom comment */
void evaluate(Context ctx) {
  ...
}

It becomes:

namespace xod {
template < /* constant inputs */ >
struct some__fqdn__ns {
    struct State { int xMemo; };
    using Type = uint32_t;
    /* Bla bla I’m a top comment */

    // typeof_xxx defs (refers Type for custom types)
    // getState defs (refers State)
    // emitValue defs (refers Type for custom types)
    // getValue defs
    // setTimeout defs
    // etc etc

    /* Bla bla I’m a bottom comment */
    void evaluate(Context ctx) {
      ...
    }
}
};

To recall, besides the comile-time template constants, the idea to wrap some parts of the node implementation with struct is to get rid of ugly non-C++ {{ GENERATED_CODE }} and to simplify referring the state. That is, get rid of State as of enclosing structure for the persistent state.

How the current solution might be adjusted to meet the requirements? The node implementation might look like:

// The custom type definition still uses the top level
using Type = uint32_t;
void iWantToStayGlobalFunc() { ... }
/* Bla bla I’m a top comment */

// Look ma, no GENERATED code

/* Bla bla I’m a bottom comment */
defnode {
    int xMemo;
    
    void iWantToBeAMethodWithStateAccess() {
      ++xMemo;
    }
    
    void evaluate(Context ctx) {
      ...
    }
};

This might be rendered to:

namespace xod {
namespace some__fqdn__ns {
using Type = uint32_t;
void iWantToStayGlobalFunc() { ... }
/* Bla bla I’m a top comment */

template < /* constant inputs */ >
struct Node {
  // typeof_xxx defs (refers Type for custom types)
  // getState defs are not required for the new-style node defs
  // emitValue defs (refers Type for custom types)
  // getValue defs
  // setTimeout defs
  // etc etc
  
  int xMemo;

  void iWantToBeAMethodWithStateAccess() {
    ++xMemo;
  }
    
  void evaluate(Context ctx) {
    ...
  }
}
}
}

So, basically, I suggest introducing the defnode keyword which is get replaced by the transpiler with stuff that now’s called GENERATED_CODE. The substitution might be even performed by C++ compiler if we #define defnode some long long long long multiline generated code before each node context and #undef defnode after, however it could mess compiler error messages and editor highlight. I think it would be better to ensure that defnode is not within comment and perform a direct substitution on the XOD side.

The code outside the defnode stays global, only namespaced to xod / some__fqdn__ns. We can go even further and clear the global scope of any implicit namespaces asking a xoder to use a namespace to avoid conflicts:

#include <LiquidCrystal.h>

ISR(...) {
  // Interupt handler must by global
}

nodespace {
  using Type = uint32_t;
  void iWantToStayGlobalFunc() { ... }
}

defnode {
  int xMemo;
    
  void iWantToBeAMethodWithStateAccess() {
    ++xMemo;
  }
    
  void evaluate(Context ctx) {
    ...
  }
};

Might become:

#include <LiquidCrystal.h>

ISR(...) {
  // Interupt handler must by global
}

namespace some__fqdn_ns {
  using Type = uint32_t;
  void iWantToStayGlobalFunc() { ... }
}

namespace some__fqdn_ns {
template < /* constant inputs */ >
struct Node {
  // typeof_xxx defs (refers Type for custom types)
  // getState defs are not required for the new-style node defs
  // emitValue defs (refers Type for custom types)
  // getValue defs
  // setTimeout defs
  // etc etc
  
  int xMemo;

  void iWantToBeAMethodWithStateAccess() {
    ++xMemo;
  }
    
  void evaluate(Context ctx) {
    ...
  }
};
}

What do you think?

@evgenykochetkov evgenykochetkov force-pushed the read-only-types branch 2 times, most recently from cba67a8 to 22a7818 Compare August 5, 2020 22:59
Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Great work!

Found a few minor issues. They are listed below.

packages/xod-arduino/platform/patchPinTypes.tpl.cpp Outdated Show resolved Hide resolved
packages/xod-arduino/platform/patchPinTypes.tpl.cpp Outdated Show resolved Hide resolved
packages/xod-arduino/platform/patchPinTypes.tpl.cpp Outdated Show resolved Hide resolved
packages/xod-arduino/platform/program.tpl.cpp Outdated Show resolved Hide resolved
packages/xod-arduino/platform/program.tpl.cpp Outdated Show resolved Hide resolved
packages/xod-project/src/constants.js Outdated Show resolved Hide resolved
Comment on lines +6 to +14
meta {
/*
A wrapper around the stock Servo object because we need to keep some details
which the original object hides in private fields. This over-protection leads
to increased RAM usage to duplicate the data. A pull request to the original
library asking to add field read methods would be nice.
*/
class XServo : public Servo {
protected:
Copy link
Member

Choose a reason for hiding this comment

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

I think such big definitions should not belong to meta. A better pattern to demonstrate would be returning the XServo definition to the global level (perhaps, only putting in xod namespace) and leaving just:

meta {
  using Type = XServo*;
}

Comment on lines 2 to 4
struct State {
uint8_t port = 0;
static const uint8_t port = constant_input_PORT;
};
Copy link
Member

Choose a reason for hiding this comment

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

Can this canonical implementation be reworked to the new style?

Also, the port should be static constexpr, not static const

@evgenykochetkov evgenykochetkov force-pushed the read-only-types branch 3 times, most recently from 94ec6dd to b04982e Compare August 24, 2020 13:31
@evgenykochetkov evgenykochetkov force-pushed the read-only-types branch 7 times, most recently from 0956117 to 155e208 Compare September 2, 2020 12:30
Copy link
Contributor

@brusherru brusherru left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

👍

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.

3 participants