-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add support for collections and other array/collections improvements #89
Conversation
Perhaps we should jump on a call and ensure that we're not doing duplicate work? We have other projects in flight including services, actions, and nested objects. |
@hoffmann-stefan thank you so much for your contribution, I'll review this shortly. |
I'd prefer we stick to GitHub unless it's necessary, I like that the users of |
@esteve . I completely agree that we should keep this all in Github to ensure all feedback is public. @ooeygui was separating the PRs for you, to address your comment about duplicated work and the scope of the change. Now we have many overlapping dependencies. You have approved #87 which implements the same collection interfaces as this PR. There will be collision once the CI break is fixed if this is approved. There is further work dependent upon integration of these contributions. It would be great to have some clear direction in terms of how you want us to resolve changes present in one set but not another, feedback on the current PRs as we delayed much refactoring and any other guidance you have for enhancing this important project. |
@ooeygui, @shschaefer: Do you have more changes in flight then the one published in #84? Before starting to implement this PR and the folow-up one I looked at all public forks but didn't find yours at that time (mid of august). But maybe I missed it. From now on we should use public issues in this repo to coordinate which changes will be done and how they will be done.
@esteve Would prefer this as well. As we are planing to use this library in our products we will implement features as we go and need them. For now we can work with the features included in our branches. Updating the description in #90 as well to include more information over there. |
@hoffmann-stefan , thanks for your reply. As we did not see any activity in this repository, we went ahead and coalesced several different branches of work into one and exchanged emails with @esteve concerning our intent. At ROS World, we tried to make it clear to the ROS community that we would be investing in this project as it benefits all and enables many scenarios such as the MR showcase we demonstrated. @esteve has recently indicated that he did not like the combined PR, so we have started to break them apart. I can appreciate your view of how our paths crossed. At the time of your PRs, there were already outstanding PRs on the same topic. That overlap is what I would like to handle as there are many other completed works behind it. We made many of the same changes as you have, just in different order. Actions are pending, logging support, better error handling and many other upgrades are next. We have also stopped work on this project pending the resolution of these PRs as they will impact the nature of what follows. @ooeygui and I are excited that this project is receiving attention and more so that you will be building products and services based on it! We would like to support that in the most effective way while also being able to take similar dependencies in our products. As you indicated, if there are additional plans that you have, let's open up issues for each in the repository and set out a publicly available roadmap showing the work and its responsible parties. |
That's not how I remember it, and I'd dare to say not what happened. I've exchanged emails with @ooeygui for over a year, and as I understood, ms-iot's fork would get merged gradually into
From the talk I gave at ROSCon in 2018, I already talked about
Not that I don't like it, it's just that it's objectively not feasible to review a 3K+ diff. And trying to get that merged, with even changing the license, is not the best way to approach an opensource community. |
@esteve I can understand your frustrations concerning the license. Had we known this was an issue, we would not have done so. @ooeygui specifically asked me about this before I made the change. It is not "an approach to the opensource community". Our code is offered freely. As it was done separately, it felt appropriate to use a copyright not in your name. We did not change the existing license. We simply added a new open license for the files we added. We are happy to change that to the Apache license if that is what you would prefer. In regards to the support of the Hololens, our work is intended to update support for the latest Hololens 2 and to bring ROS2 forward to the new application model of HL2 as it is incompatible with the existing code from @fmrico and earlier efforts. We are not attempting to reduce the importance of your work. It will always stand as your recordings show. Our work for the Mixed Reality Toolkit is to build platform components that could be used in ARViz should you choose to update it. We are not building an AR application to compete with ARViz. My question stands as to how you would like to merge the overlapping code. We will ensure that the license issue is resolved to your satisfaction, representing the work that has been done and will be in the future. One example solution is to add an attribution block to the README or another location to ensure that Stefan and other's contributions are noted. Using the scope of change as a metric, PR #90 is over 2500 lines. The services code requires a large amount of boilerplate that accompanies the core of the work. No matter which PR is accepted, the scope is significant if we simply look at LOCs. The action support code will require some boilerplate for P/Invokes and other areas as well. I can understand separating TF changes, collections, services, etc... Which is what @ooeygui was in process of doing at your request. Please let us all know how you would like to proceed. We will hold off any further changes until the repository converges. We continue to support the principles of ROS2 for .Net and look forward to contributing to your project in the future. |
I asked you about that, but the response I got was that Microsoft would only release as MIT. If you guys can change your contributions to have the same license as the rest of
#90 depends on #89, so it includes changes from both PRs so the actual diff is smaller, but in any case, both have a clear focus. Instead #84 includes many features, some of them unrelated from each other, so it's all or nothing, features can't be merged one by one.
I understood that @ooeygui was working on the build issues (as per #87 (comment)), I've approved #87 pending the build fixes, but I don't know what's the progress status on that. Anyway, let's better continue this conversation in another ticket, this is taking away the focus from @hoffmann-stefan 's contribution. |
Named {member}_Count as this is the normal .NET lingo. removed getsize_array_field c function as well as it is constant, no need for interop
Idl arrays now map as System.Array instead of List<T>. This is the more natural C# type for them.
- size gets checked on writing to the native handle of the message. - change foreach to for to avoid multi threaded race conditions while writing to native memory. (the upper bound is now local and cannot be changed from somewhere else)
`List<T>.Count` is of c# type `int`, wich is 32 bit signed. C does use `sizeof_t` wich is 32/64 bit dependent. So this does marshal at least correctly, ignoring cast from 64 bit to 32 bit and vice versa for now. getting marshaling/conversations 100% correct is anoher issue for another day.
929fc25
to
7dc4f12
Compare
@esteve, @ooeygui, @shschaefer: Since CI is now passing in this PR and in #90 how is the plan about reviewing these PRs? Just wanted to do a reminder/ping here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hoffmann-stefan thanks!
@esteve thanks for merging! One question: did you intentionally squash all commits into one when merging? |
This PR was developed without knowing that @ooeygui / ms-iot (pr #87) did similar things.
As this is our (as in Schiller Automatisierungstechnik GmbH) first open source contribution, there was some time between writing the code and now publishing and discussing this in the open ;)
Features added
Add support for arrays/collections of strings
Add support for collections (unbounded and bounded)
Add size constants for arrays and bounded sequences
Change mapped class for arrays
Ros arrays are better described with
System.Array
than withList<T>
.For example, adding to an array doesn’t make sense as it would violate the length check on publish.
Initialize arrays with default values
In
rclpy
anrclcpp
the default is to initialize all members, including arrays.Add bounds checks for arrays and bounded sequences
Arrays of wrong size and bounded sequences that exceed the maximum length will throw
an exception on publishing the message.
Added Tests for all new features
Nothing more to say ;)
Follow-up adding support for Services
I did continue with implementing services as well. Opend a draft PR #90 which is based on this one.
It will include a fix to
get_error_string
as well, which is the only feature that #87 has added which isn't included in here as far as I reviewed it.