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

New Virmata plugin integration #49

Merged
merged 17 commits into from
Aug 15, 2012

Conversation

jnslxndr
Copy link
Contributor

Hi all,
chris and i were discussing to integrate the new firmata plugin into the addonpack and as i wrote most of it and am the git-guy it seems my task.

So after nearly two days of windows fight the sdk compiles fine and the addonpack (nearly, one cc+ plugin can't find some pref file) did build too. i followed the instructions on the site, though it did not really work out of the box, but a good guide nevertheless. and i love my unix commandline...

Ok, well, yes, this is the first contribution integration i am doing, so can you have a look at it and give feedback, if anything does/will/might not work.

I also deleted the old cpu spikey plugin and provided a legacy node. I sthis the proper way id did it? I was not able to get my hand on some old patches to thoroughly test it. I also thought about splitting up the commits to have the removal of the old plugin extra - needed?

Am i missing something? Which planed release would be good to ship it with? Wait for the beta28?

cheers! jens

ps. Did you think about using git submodules for plugin integration?

@joreg
Copy link
Member

joreg commented Apr 29, 2012

hola jens,

thanks for that. i made some comments concerning naming directly in the code.

regarding the legacy modules i am not sure. we typically tend to leave old plugins as they are, just add a version "Legacy". in addition to setting the plugin legacy you'll have to add an entry to \lib\diffff.xml. like that the plugin will no more show up in the nodebrowser but old patches will still load that plugin and behave exactly as before. see? so typically no extra legacy modules needed..or is there something about those that i didn't get?

yes, this will be included automatically with b28
and yes, we've thought about using git submodules and came to the conclusion that they'd cause a big/unnecessary overhead for everyone.

would be interesting to hear what didn't work out of the box when you tried to get the sdk to build because basically it should, except the one c++ plugin that is a known problem, but nothing to worry about. so i'd love to improve the documentation if you have any comments (but probably that fits better in the vvvv-forum...)

@jnslxndr
Copy link
Contributor Author

hi joreg,

thanks for quick response. I read the comments and shame on me, yes, that will be fixed. #region blindfolded me....

Thanks for the Legacy explanation. This totally makes sense and is exactly what i wanted. As far as i understood it from Chris a proper Legacy tagged module should be made. I will re-add the old plugin, adjust the help file and the diffff.xml.

As the addition will be in the beta28 addonpack; do you have any dately pointers, when this will be? i could try to round up the encoder with I2C encoding as well (should be too much work, but time is an issue).

And yes, of course, i don't rant and provide no nothing to the "out of the box", i will have to set my VM back then i can target the exact bummers i experienced.

thx!jens;

@jnslxndr
Copy link
Contributor Author

hi ioreg,
i am not sure, if i missid anything of the changes you commented. I would love to have some more tests on this before it is merged. unfortunatly this wil be monday. but any feedback is welcome, espacially on the difff.xml! i do not know, if i did it right!?

gr!j;

@jnslxndr
Copy link
Contributor Author

jnslxndr commented Jun 1, 2012

i had some thoughts on the "versioning", without really making decision yet. you are not getting it wrong, but i am not sure, if it fits to the overall roadmap i'd like to take for the plugin. There are also Firmata based Firmwares which set a version of 0.0 or 0.2 (AnalogIn and the Servo only firmwares afaik).

a sketchy thought is to put an inpektor-only pin in to choose which major version to encode/decode. this would make sense, as I am already trying to keep firmata specifics in a seperate class, so it can be reused elsewhere too.

maybe i am overcomplicating things too and should just get it done for version 1.0 (aka givin it the proper version attribute of 2.x)!? Am I correct, that the "Version" attribute only relates to node naming?

@joreg
Copy link
Member

joreg commented Jun 1, 2012

having a config-enum that lets the user choose a firmata-version sounds good too but it would only make sense if with changing that version no input or output pins change. else i'd say extra nodes for each version are better.

in the case of using the enum you'd leave the nodes "Version" blank, else use the firmata version.

jens-a-e added 3 commits June 26, 2012 16:48
After some thought, it seems ok and logical to stick to versioning
scheme of the major versions of the firmata protocol.
@jnslxndr
Copy link
Contributor Author

Ok. I gave it some thinking, then found some time, gave chris a pre-release package for the course, got good feedback and fixed some minors and now, i i think i can show it to you :)

greets to berlin!

@jnslxndr
Copy link
Contributor Author

Oh, yes; it is versioned with regard to the Firmata versioning scheme. So as it supports all 2.x protocols, it has this version number.

@joreg
Copy link
Member

joreg commented Jun 26, 2012

k, so do i get this right?
we get 2 new plugins:
FirmataEncode (Devices 2.x) & FirmataDecode (Devices 2.x)
and a new module:
Arduino (Devices StandardFirmata 2.x)

and the old plugin:
ArduinoFirmata (Devices)
will be legacy from now on.

in that case it seems to me that the old node is still missing its "Legacy" version. while you added that to the diffff.xml you still need to add it to the actual nodes info. this will make that node no longer show up in the NodeBrowser.

@jnslxndr
Copy link
Contributor Author

Ok. Then i did not get the instructions for legacylizing a node proprly. Will add it asap.

@jnslxndr
Copy link
Contributor Author

Yes, the Plugin is split into 2, actually 3, Nodes one for encoding, one for decoding a firmata message stream. This strategy decouples the protocol parsing and packing from its device - so dreaming of firmata over tcp/udp/wysiwyg/etcpp is possible as long as the string encoding is kept to the nice ANSI :)

@joreg
Copy link
Member

joreg commented Jul 17, 2012

so i am still waiting for the update here, right?

@jnslxndr
Copy link
Contributor Author

hi joreg,

hm. as far as i am concerned for the first release, this should be it. my anserws might have been confusing. The plugin contains the 3 plugin + one module for easy direct use. i patched the difff.xml for version b28 to deprecate the old plugin - which i was confused with at first. the I2C and further ShiftRegister support i will add in the next cycle. the old plugin is so hungry and inefficient on the CPU, which makes it first candidate to fix and the current state covers the features + adds some. if i horribly missed something or ignored any conventions, shame on me and i am happy to double check.

if i got something wrong, please help me out to remember.

@joreg
Copy link
Member

joreg commented Aug 6, 2012

still waiting..

@jnslxndr
Copy link
Contributor Author

... sorry for the delay! i just couldn't get it done and in the meantime i had to travel again. now it should be done(!? was the correct spot, right?) and fixed an annoying buggy startup too.

i tested against the current develop & vvvv.exe branch and had no complaints.

i am much better available again for the next couple of months.

joreg added a commit that referenced this pull request Aug 15, 2012
@joreg joreg merged commit 3a12b10 into vvvv:develop Aug 15, 2012
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.

2 participants