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

C# API again #129

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

C# API again #129

wants to merge 17 commits into from

Conversation

pgrudzien12
Copy link

Currently implements:

  • generic sensor class
  • large and medium motor
  • battery

Whenever possible constants, basic methods and comments are generated with liquid.

I had to change camel_case as 7e098d2 changed behavior for undefined values.

@pgrudzien12 pgrudzien12 mentioned this pull request Nov 29, 2015
@WasabiFan
Copy link
Member

I had to change camel_case as 7e098d2 changed behavior for undefined values.

That's great 😉 -- sorry I changed it without making sure it behaved as before! Thanks for taking the time to fix my mistakes.

Currently implements: ...

It looks good to me. You'll probably want to implement those others at some point, but it's a lot to deal with at once and we still haven't even gotten all of our current code caught up with the new classes so it's completely understandable.

Whenever possible constants, basic methods and comments are generated with liquid.

Overall, how'd that go? I would hope that it made it easier for you to get started, but I know that we don't have much in the way of "getting started" docs or info on how to configure autogen; we definitely don't want it to feel like a chore. What do you think we need to improve to make it as easy as possible for new people to develop bindings? And were the templates tiresome to create (figuring out errors, formatting whitespace, etc.) or was it manageable? Additionally, given that you had to hunt down that autogen bug, how easy was it to find? I'm just looking for honest feedback, so feel free to be blunt.

A few notes:

  • I like your use of partial classes! It gives some nice separation between components and clearly distinguished between autogen and freehand.
  • It would be great if you added C# to the list of languages on repo README.
  • On (most of) our current libraries, we have a README.md file in the subfolder for each language with a brief description of the library, information on compilation/usage and anything else that is important for someone to see. It doesn't need to be more than a sentence or two if there's not much that you think is needed, but please add one in the csharp folder with at least the information on how you are building/running your code.
  • Currently, this PR will add the code directly into the ev3dev-lang repo. This is completely fine, but given that you will be the primary maintainer of it, it might be easier for you if you instead create your own repo (ev3dev-lang-csharp) under your account and just add a submodule that references that repo which we update when needed.
    • See the python and js submodules for examples; the repos are under @rhempel's and my accounts, and we each push to those with our code and then just update the reference here. With this model, we can update your submodule on request without you being gated on us while developing.
    • If you decide to do this, the current PR would still include your autogen changes (camel_case, autogen-list.json, etc.) but it wouldn't actually include the C# code -- it would just have the submodule.

I'm going to give this a day to make sure that @ddemidov gets a chance to look at it, but after that I'll merge it. Let me know when you're done making any changes that you want to make.

@ddemidov
Copy link
Member

I have little to no experience with C#, so I can not comment on the code :). In addition to @WasabiFan comments, it would be nice to have a basic example (or two) of using the library. The examples may be based e.g. on Explor3r robot by Laurens Valk (Laurens was kind enough to explicitly allow using the robot instructions for this purpose). Having a working example would provide an immediate overview of the library possibilities.

@pgrudzien12
Copy link
Author

Hey thanks for your interest. It's really nice to see that people are so engaged as you are in their side projects.

@WasabiFan I'll give you my feedback in a few days - I just don't have much free time besides weekends.

@ddemidov I agree that it would be good to have example. I will see into that after improvements to library itself.

@pgrudzien12
Copy link
Author

@WasabiFan

You'll probably want to implement those others [features] at some point

You are right. I'll try to do as much as possible.

Overall, how'd that go?

It was okish. Readme.md file under autogen helped a lot at the beginning. After this initial learning period I was able to use cpp lang and others as examples and I have to say that this part was pretty smooth.

The worst part I think was liquid syntax itself. After it goes beyond 10-15 lines of template it becomes unmanageable and needs rethinking. It would be helpful to have a feature in liquid like calling generation of part from within working generator. Ending lines with %}{% felt very uncomfortable and clutters template a lot. I think that it's not well fitted into this kind of generating code. It is ok for html or places where amount of code exceeds template code but I ended up with exactly opposite ratio. However I don't know any template engines that would meet your needs.

I think that having spec.json file is a great idea. I felt that some of the parts that are not currently autogenerated could be moved into this file as well eg. some constants like in1, out1.

Additionally, given that you had to hunt down that autogen bug, how easy was it to find?

Finding it wasn't a problem. It made me think about my code if I should detect undefined values beforehand. Sadly I don't know how to handle it in liquid.

It would be great if you added C# to the list of languages on repo README

I will. Hopefully today.

... brief description of the library, information on compilation/usage ...

Yes I feel that without it even fairly determined folks wouldn't like to use it. It's a must.

just add a submodule that references that repo

Hell I don't know yet how to do it but it seems reasonable. I'll look into this soon. If it require you to reject PR then don't hesitate. I'll make another if needed after catching up with this idea.

@WasabiFan
Copy link
Member

The worst part I think was liquid syntax itself.

I definitely agree and feel your pain -- liquid isn't very good at generating code in which you're trying to manage whitespace. I had been looking for alternate templating engines, but there really aren't any that do what we need as far as I can tell. And obviously, transitioning the templates that we have now would be a pain -- but it might be worth it if we could find a good alternate system.

Sadly I don't know how to handle it in liquid.

That's why I added the extra methods in that definition file for autogen -- my hope is that you don't have to implement any complex logic in liquid. Feel free to add autogen functions whenever there is more involved functionality that you need.

If it require you to reject PR then don't hesitate.

Definitely not! I have no problem with you including the files directly, and if you haven't used submodules I would recommend against them at the moment. They are pretty complex and don't provide much benefit for most things. So just make the changes that you want to on this PR and we'll merge it as-is.

@ddemidov
Copy link
Member

ddemidov commented Dec 5, 2015

I had been looking for alternate templating engines, but there really aren't any that do what we need as far as I can tell.

Have you been looking outside of javascript? We could use same spec.json with some other scripting language, like python. I don't have much experience with text templates, but quick search came out with e.g. http://www.cheetahtemplate.org/, and I am sure there are others.

@rhempel
Copy link
Member

rhempel commented Dec 5, 2015

The nice thing about a templating engine is that we can replace it :-) As ling as we're based on spec.json we're all good.

@pgrudzien12
Copy link
Author

Maybe mustache? https://github.com/janl/mustache.js. It is widely used.

@WasabiFan
Copy link
Member

We should open a new issue if we want to discuss the templating language.

@pgrudzien12
Copy link
Author

It took me a week but i finally made it:

  • moved code into projects
  • implemented support for many simple sensors (some of them will hopefully later get some more specific getters)
    • I2cSensor
    • ColorSensor
    • UltrasonicSensor
    • GyroSensor
    • LightSensor
    • TouchSensor
  • implemented IrRemote - demo that acts as default remote that comes with ev3
  • added demo code
  • updated global README.md + added README.md stubs for projects

C# code is still a little slow on startup but I'm looking forward for RTM version of DNX/dotnet that will add native compilation feature.

Let me know if this is enough for you to merge it.

I'd like to upload packed version of ev3dev up to nuget.org to ease development. What do you think about that? I'd call it simply ev3dev if you don't mind. If you would suggest better name for it just let me know.

@rhempel
Copy link
Member

rhempel commented Dec 12, 2015

I think that we may need to be a little more protective of ev3dev as a standalone word or package name. In my mind ev3dev is the term we are using for the entire distribution - and it's a little wierd running ev3dev on other platforms, but the intent is to support LEGO compatible shields on those platforms.
So I'd prefer if you named your binding ev3dev-csharp or something like that - whatever you like - but please not just ev3dev by itself to avoid confusion.
Anyone else have any comments for or against?

@dlech
Copy link
Member

dlech commented Dec 12, 2015

I'd call it simply ev3dev if you don't mind\

If you are writing a library that adheres to the ev3dev-lang spec, you should use the namespace (and package name) Ev3devLang. Also see the discussion in #133.

Also, as a potential contributor to the C# language bindings, I would prefer that you maintained the library as a submodule rather than in the ev3dev-lang repository directly. This way, I can just clone the ev3dev-lang-charp repository (or whatever you choose to call it) instead of having to clone all of ev3dev-lang.

@rhempel
Copy link
Member

rhempel commented Dec 12, 2015

Yup, that's the way we've been doing ev3dev-lang-python, and how I plan to finish of ev3dev-lang-lua

@pgrudzien12
Copy link
Author

Ok I see your concerns regarding ev3dev package/namespaces. There's no reason to worry about that. I'll do quick rename to avoid any confusion and misunderstandings.

@dlech Also if you prefer to have it as a submodule then I'll look into that and see what I can do.

@dlech
Copy link
Member

dlech commented Dec 12, 2015

There are some tips on submodules in #30.

@rhempel
Copy link
Member

rhempel commented Dec 16, 2015

I am going to hold off on merging this until we (@dlech) get the next image out the door.

@pgrudzien12
Copy link
Author

As requested I've replaced code with submodule that will be maintained at https://github.com/pgrudzien12/ev3dev-lang-csharp. This repo contains all the csharp specific code.

I've also renamed namespace to meet your needs.

If there is anything I can do let me know.

@WasabiFan
Copy link
Member

@pgrudzien12 We recently changed the structure of our repo to make it so that, aside from adding your language to the list of language comment formats, you don't have to modify the main repo to be able to add and autogen your code. See #142 for more information.

This means that you don't need to deal with your code inside a submodule after all -- sorry for the extra work and long wait!

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.

5 participants