Skip to content

Conversation

@peternewman
Copy link
Member

Hi @JanOveSaltvedt ,

I see you've added support for your ovdmx dongle to OLA and for the dongle here:
https://github.com/JanOveSaltvedt/ovdmx

We can get this merged into OLA mainline if you'd like as you seem to have a little bit of a tree of users of your codebase:
https://github.com/JanOveSaltvedt/ola-ovdmx/network/members

From a quick look the initial code doesn't look too far off being ready to merge.

@peternewman peternewman added this to the Future milestone Sep 6, 2019
@peternewman
Copy link
Member Author

This commit probably wants adding too: JanOveSaltvedt@5b95972

@aasmundeldhuset are you interested in helping to get this merged too?

@peternewman
Copy link
Member Author

Or @stemnic

@stemnic
Copy link

stemnic commented Sep 10, 2019

I think it would be nice to have ovdmx integrated with the main branch of OLA.
As @JanOveSaltvedt said the pull should be set to 0, 0 which currently matches the default firmware. However, I think the plugin needs some work though before it can be merged with OLA since I don't think it currently compiles with the latest version without errors

Copy link
Member Author

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

You also need to add the licence to the top of some files:
https://travis-ci.org/OpenLightingProject/ola/jobs/581847612#L1041-L1045

Fix some lint issues (although that's normally best done last):
https://travis-ci.org/OpenLightingProject/ola/jobs/581847611#L1635-L1642


std::string Name() const { return PLUGIN_NAME; }
std::string Description() const;
ola_plugin_id Id() const { return OLA_PLUGIN_EXPERIMENTAL; }
Copy link
Member Author

Choose a reason for hiding this comment

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

@peternewman
Copy link
Member Author

The initial compile failure on Linux is fairly simple @stemnic , just see the notes here:
https://github.com/OpenLightingProject/ola/blob/master/common/protocol/Ola.proto#L72-L78

@stemnic
Copy link

stemnic commented Sep 10, 2019

Great I will take a look at it and see if I can make it work

@peternewman
Copy link
Member Author

Great I will take a look at it and see if I can make it work

Brilliant thanks. I think it's probably better if you re-open the PR because then I can approve it for merge (and you are a LOT more involved in the codebase than me, so I'll close this one so it lets you open a fresh one; at least I think it will then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants