| Message ID | 20210319164046.5626-1-m.cichy@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hey Marian, Thank you for the patch. On 19.03.2021 17:40, Marian Cichy wrote: >Informations how to identify Controls from a ControlList is quite s/Informations how/Informations on how/ s/is quite/are quite/ >scattered around the documentation and not clear if one reads about the s/and not clear/and it is not clear/ >ControlList. Referring to ControlId and ControlIdMap right in the >detailed description is probably very helpful. agreed :) > >Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> >--- > src/libcamera/controls.cpp | 4 ++++ > 1 file changed, 4 insertions(+) > >diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >index c58ed394..b5253f83 100644 >--- a/src/libcamera/controls.cpp >+++ b/src/libcamera/controls.cpp >@@ -805,6 +805,10 @@ ControlList::ControlList() > * For ControlList containing libcamera controls, a global map of all libcamera > * controls is provided by controls::controls and can be used as the \a idmap > * argument. >+ * >+ * To identify a Control from the ControlList, one needs to find the ControlId >+ * from the numerical control id saved in this list. A global ControlIdMap >+ * of all libcamera controls is provided by controls::controls. This sentence seems to be very similar to the sentence two rows above. How about: ``` * For ControlList containing libcamera controls, a global map of all libcamera * controls is provided by controls::controls and can be used as the \a idmap * argument. * In order to identify a single control from the ControlList, one needs to find * the ControlId using the contains method in the numerical control IDs of that * map. ``` I feel like when we are at it, just mention how you are supposed to do it. > */ > ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator) > : validator_(validator), idmap_(&idmap), infoMap_(nullptr) >-- >2.29.2 Greetings, Sebastian > >_______________________________________________ >libcamera-devel mailing list >libcamera-devel@lists.libcamera.org >https://lists.libcamera.org/listinfo/libcamera-devel
Hi Marian, Thank you for the patch. On Fri, Mar 19, 2021 at 06:03:57PM +0100, Sebastian Fricke wrote: > On 19.03.2021 17:40, Marian Cichy wrote: > > Informations how to identify Controls from a ControlList is quite > > s/Informations how/Informations on how/ > s/is quite/are quite/ > > > scattered around the documentation and not clear if one reads about the > > s/and not clear/and it is not clear/ > > > ControlList. Referring to ControlId and ControlIdMap right in the > > detailed description is probably very helpful. > > agreed :) > > > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> > > --- > > src/libcamera/controls.cpp | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index c58ed394..b5253f83 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -805,6 +805,10 @@ ControlList::ControlList() > > * For ControlList containing libcamera controls, a global map of all libcamera > > * controls is provided by controls::controls and can be used as the \a idmap > > * argument. > > + * > > + * To identify a Control from the ControlList, one needs to find the ControlId > > + * from the numerical control id saved in this list. A global ControlIdMap > > + * of all libcamera controls is provided by controls::controls. > > This sentence seems to be very similar to the sentence two rows above. > How about: > ``` > * For ControlList containing libcamera controls, a global map of all libcamera > * controls is provided by controls::controls and can be used as the \a idmap > * argument. > * In order to identify a single control from the ControlList, one needs to find > * the ControlId using the contains method in the numerical control IDs of that > * map. > ``` > > I feel like when we are at it, just mention how you are supposed to do > it. Furthermore, usage information shouldn't be documented in one particular constructor, but at the class level, or even file level. I think file level would be more appropriate here, to tie together Control, ControlId and ControlList. > > */ > > ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator) > > : validator_(validator), idmap_(&idmap), infoMap_(nullptr)
On 3/19/21 6:03 PM, Sebastian Fricke wrote: > Hey Marian, > > Thank you for the patch. > > On 19.03.2021 17:40, Marian Cichy wrote: >> Informations how to identify Controls from a ControlList is quite > > s/Informations how/Informations on how/ > s/is quite/are quite/ > >> scattered around the documentation and not clear if one reads about the > > s/and not clear/and it is not clear/ > >> ControlList. Referring to ControlId and ControlIdMap right in the >> detailed description is probably very helpful. > > agreed :) > >> >> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> >> --- >> src/libcamera/controls.cpp | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index c58ed394..b5253f83 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -805,6 +805,10 @@ ControlList::ControlList() >> * For ControlList containing libcamera controls, a global map of all >> libcamera >> * controls is provided by controls::controls and can be used as the >> \a idmap >> * argument. >> + * >> + * To identify a Control from the ControlList, one needs to find the >> ControlId >> + * from the numerical control id saved in this list. A global >> ControlIdMap >> + * of all libcamera controls is provided by controls::controls. > > This sentence seems to be very similar to the sentence two rows above. > How about: > ``` > * For ControlList containing libcamera controls, a global map of all > libcamera > * controls is provided by controls::controls and can be used as the > \a idmap > * argument. > * In order to identify a single control from the ControlList, one > needs to find > * the ControlId using the contains method in the numerical control > IDs of that > * map. > ``` > Not sure, why you would recommend contains() here, what are you trying to achieve? contains() won't give me any information about the mapped ID in any direction, right? As an example, I'd write a snippet to merely iterate over all items in ControlList and use each numerical ID with controls::controls.at(). This was for me my first approach when I wanted to print the names of all my controls. Regards, Marian > I feel like when we are at it, just mention how you are supposed to do > it. > >> */ >> ControlList::ControlList(const ControlIdMap &idmap, ControlValidator >> *validator) >> : validator_(validator), idmap_(&idmap), infoMap_(nullptr) >> -- >> 2.29.2 > > Greetings, > Sebastian >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index c58ed394..b5253f83 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -805,6 +805,10 @@ ControlList::ControlList() * For ControlList containing libcamera controls, a global map of all libcamera * controls is provided by controls::controls and can be used as the \a idmap * argument. + * + * To identify a Control from the ControlList, one needs to find the ControlId + * from the numerical control id saved in this list. A global ControlIdMap + * of all libcamera controls is provided by controls::controls. */ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator) : validator_(validator), idmap_(&idmap), infoMap_(nullptr)
Informations how to identify Controls from a ControlList is quite scattered around the documentation and not clear if one reads about the ControlList. Referring to ControlId and ControlIdMap right in the detailed description is probably very helpful. Signed-off-by: Marian Cichy <m.cichy@pengutronix.de> --- src/libcamera/controls.cpp | 4 ++++ 1 file changed, 4 insertions(+)