New proposal for SensorID List #161
Conversation
|
@CarloVanDriestenBMW It was faster to add a new PR instead of merging the master in the old PR. I hope it is ok.... |
|
This PR addresses issue #147. |
ghost
added
FeatureRequest
|
I guess we have to define how the version of the ISO standard and the version of OSI is combined. E.g. an entry in the readme.md in order to clearly define to which ISO this specific OSI version relates to? |
|
I agree, but we can't refer to this standard (there is no number). The standard will define the communications and if we add semantic compatibility (as a restriction to minor version changes) I think we are prepared. |
|
@CarloVanDriestenBMW: last changes have no renumbering of fields. We should do this at last.... |
pmai
force-pushed
the
Detected--message-references-sensors-V2
branch
from
7cefe02 to
12ac0c5
Compare
|
@carsten-kuebler, @CarloVanDriestenBMW I've rebased onto master and squashed the fixup commit, so please git checkout Detected--message-references-sensors-V2 && git fetch && git reset --hard origin/Detected--message-references-sensors-V2 prior to further work on this branch. Generally-speaking, is it wise to seperate the Header from the Objects like this? This requires a more complex traversal for everyone, AFAICS, with limited benefit? I.e. having a generic Header message and including that in all DetectedXXX seems like a more direct approach, or am I missing something? |
|
@pmai The header is not for one detected entity. It is for all objects of one detected entity . Maybe it is easier to understand, if we use plurals for field names. |
pmai
force-pushed
the
Detected--message-references-sensors-V2
branch
from
d748b97 to
12ac0c5
Compare
|
@carsten-kuebler I've re-reset this branch, since a merge commit which included all the old commits reappeared: Please be sure that after a branch reset you do not do a git pull on the branch, but rather a git fetch and git reset --hard origin/branchname, to take up the rebased stuff. The reason I do these rebases is to cleanup history prior to merging into mainline (i.e. avoid fixup commits, multiple merges between branches, conflicts, etc.), to keep development history usable going forward. |
|
@carsten-kuebler On the object headers: yes, the naming and old usage fooled me; if the data is really only per entity kind, we should somehow make this a bit clearer in the names and/or documentation, I think... Otherwise looks good to me... |
pmai
force-pushed
the
Detected--message-references-sensors-V2
branch
from
12ac0c5 to
619b2d5
Compare
|
@carsten-kuebler, @CarloVanDriestenBMW I have re-rebased to solve the conflicts with newly merged master as best I could; please re-check if this still matches what is wanted. I'd suggest to merge these content PRs prior to doing major changes of documentation and/or field renumberings, since this will create lots of spurious conflicts... |
pmai
force-pushed
the
Detected--message-references-sensors-V2
branch
from
141d2ad to
619b2d5
Compare
Test
As discussed, I moved DetectedObjectHeader to Sensor Data. I didn't number consecutively (easier to merge). We have to renumber at the end...
pmai
force-pushed
the
Detected--message-references-sensors-V2
branch
from
619b2d5 to
2d08aa5
Compare
|
@CarloVanDriestenBMW is conflict free and should be mergeable now; approve if this works for you. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Some typos in the documentation. I think the names are a clearer now
osi_detectedlandmark.proto
Outdated
| // A list of candidates for (a) possible supplementary sign(s) as estimated by the sensor. | ||
| // | ||
| // A list of candidates for (a) possible supplementary sign(s) as estimated | ||
| // by the sensor. |
osi_sensordata.proto
Outdated
| // The ID of the sensor at host vehicle's mounting_position. | ||
| // | ||
| optional Identifier id = 1; | ||
| // This ID can equal \c DetectionHeader::sensor_id, if SensorData hold only |
|
@CarloVanDriestenBMW I've fixed and regularized the Doxygen comments. |
ghost
merged commit 
New proposal for PR #151