OpenSimulationInterface
Feature/tp/had output
Added osi_motionrequest.proto file containing a MotionRequest message. The message is intended as an interface between a HAD function and the actuator managment. The current version of the version implemented contains options for DesiredState and a future trajectory. The implementation uses the changes made in pull request 401 "Add future trajectory to moving object".
Check the checklist
- [x] My code and comments follow the style guidelines and contributors guidelines of this project.
- [x] I have performed a self-review of my own code.
- [x] I have made corresponding changes to the documentation.
- [x] My changes generate no new warnings.
- [ ] I have added tests that prove my fix is effective or that my feature works.
- [ ] New and existing unit tests / travis ci pass locally with my changes.
@lemmer-fzi I tried switching the base so this PR only showed the changes that were relevant to this (i.e. just the motion request file) but it still seems to pick up all the commits from the PR401 branch, so I suspect it is because the PR401 branch has since been rebased onto a new master.
I'm happy to sort that out, but it would be easiest to do it by rebasing this branch which is a slightly destructive option so don't want to break anything you've got locally.
@caspar-ai If you want to go ahead and rebase the branch to make the changes more clear that is fine for me.
@lemmer-fzi all done.
- rebased the branch onto the latest WP-10 branch
- updated the PR base branch to be the WP-10 branch so the changes tab only shows the changes relating to this PR
- fixed up the sign-off check
There is still some odd indentation, but probably not worth worrying about too much now.
@caspar-ai While the oneof makes the intent clearer, we are currently avoiding oneof (see the Actions in TrafficCommand for another instance where oneof might have been used but was backed out), since it has a number of backward/forward compatibility issues on ProtoBuf, is on the wire equivalent to multiple optional fields anyway, and currently cannot be auto converted to flatbuffers.
I think in this case two optional fields with documentation stating that only one of them shall be used at any one time would be sufficient.
@pmai interesting, thanks. I was just trying to avoid reinventing the wheel, but happy to stick with the long-hand way of doing it. I've reverted that change.
Output from CCB meeting - 13.01.2021 Actions:
- @Krishna626 Review osi_motionrequest.proto file, namely the Trajectory and DesiredState; they probably should be a part of MotionRequest and not the top level message (this is only an observation).
Output from CCB meeting - 20.01.2021 Actions:
- @max-rosin Documentation Review to be done (including some additional spaces to be removed).
- Edited PR Merge to master instead of WP10. Requried dependency (StatePoint Message) is already integrated in master/osi_common.proto.
- Merge should able to work. Conflicts can safely be resolved. -- PR intention is to add a new file (osi_motionrequest.proto) to master
- Unable to push local changes (Rebase to master + conflicts resloved) to remote. Permission got denied. -- Will be tried again on monday with @ThomasNaderBMW
- @lemmer-fzi: For your information.
@Krishna626 I've done the rebase and pushed the updated branch. A few comments:
- When you didn't have permissions did you do
git push --force? You need--forcebecause you have rewritten history, and you need to understand the implications of doing that (as in, anyone else with that branch checked out will have to delete it and re-check it out) - It picked up a couple of minor docs fixes in other files, presumably @paagkame made those in commits mostly fixing motion request docs
- I tidied up the "Signed off" lines to add yours to the most recent 3 commits @Krishna626 hope that's OK
- @pmai there are errors on the DCO check - every commit is signed off but seemingly not with the precise names / emails people are expecting. How would you suggest is the best way of fixing? Manually fixing up every commit to have the GitLab expected names / emails - even if they are a bit "odd", e.g. from when Krisha made commits in the web IDE?
@caspar-ai Thanks for your efforts. I have done the signoff for all the commits modified and tried again to push to the branch with the instructions mentioned in DCO. Again i get a message, that permission denied to push to the branch. @pmai What am I missing to bring my local changes to the remote? Surprisingly, I able to commit through IDE/Web browser. Can you please help me out with this? Thanks!
git rebase HEAD~10 --signoff Now your commits will have your sign off. Next run git push --force-with-lease origin feature/tp/had-output
Output from CCB meeting 17.02.2021 Actions:
- Pushed to v3.4
- @caspar-ai @Krishna626 please include a documentation for the MotionRequest message following the example for traffic participants in the OSMP.
- @caspar-ai @Krishna626, we should also include such a documentation in the OSI official documentation (align with @max-rosin )
Output from CCB meeting 17.02.2021 Actions:
1. Pushed to v3.4 2. @caspar-ai @Krishna626 please include a documentation for the MotionRequest message following the example for traffic participants in the [OSMP](https://github.com/OpenSimulationInterface/osi-sensor-model-packaging/blob/feature/osi-traffic-participants/doc/specification.rst#sensor-view-input-configuration). 3. @caspar-ai @Krishna626, we should also include such a documentation in the OSI official documentation (align with @max-rosin )
First draft for specification
@kmeids What is missing to process this pull request? Can I set it to ready for ccb review?
@kmeids What is missing to process this pull request? Can I set it to ready for ccb review?
if the following notes https://github.com/OpenSimulationInterface/open-simulation-interface/pull/452#issuecomment-796676964 are handled then yes you can set it to CCB review
FYI. This PR is a likely candidate to be dropped from V3.4. Otherwise get in touch! @lemmer-fzi @ThomasNaderBMW @paagkame @kmeids
FYI. This PR is a likely candidate to be dropped from V3.4. Otherwise get in touch! @lemmer-fzi @ThomasNaderBMW @paagkame @kmeids
What is still missing? I guess we did the necessary steps? Or do I only need to mark it as CCB Rebview ?
FYI. This PR is a likely candidate to be dropped from V3.4. Otherwise get in touch! @lemmer-fzi @ThomasNaderBMW @paagkame @kmeids
What is still missing? I guess we did the necessary steps? Or do I only need to mark it as
CCB Rebview?
Are the notes #452 (comment) handled yet? I don't see a commit.
The commit/branch I mentioned here solves the OSMP documentation issue.
https://github.com/OpenSimulationInterface/open-simulation-interface/pull/452#issuecomment-796676964
Max rosin commited something about the documenation on August 9
The commit/branch I mentioned here solves the OSMP documentation issue. #452 (comment)
In the meantime we totally restructured the documentation. OpenSimulationInterface/osi-sensor-model-packaging#63 would need to be redone completely.
Max rosin commited something about the documenation on August 9
Not related to the comments from the CCB.
@kmeids Please decide what to do with this PR. If the project wants this in 3.4 we would need to merge in the next 2-3 days.
Sorry for not updating on here earlier, but I'm not really involved in this PR other than to help with git admin :)
I was therefore leaving it for @Krishna626 to pick up, but I believe he may be doing less OSI related work these days.
@lemmer-fzi, Are you still working on this feature request? Is there still an interest in adding the "MotionRequest" message?
At this moment i am no longer working on this pull request, as from my point of view the actual content is finished. If their is still interest for the provided content i'm willing to fix technical issues.