Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Sat, Jul 25, 2009 at 04:39:18PM +0200, ext Hans Verkuil wrote: > On Saturday 25 July 2009 15:25:21 Eduardo Valentin wrote: > > On Sat, Jul 25, 2009 at 03:33:55PM +0200, ext Hans Verkuil wrote: > > > On Saturday 25 July 2009 15:29:38 ext-eero.nurkk...@nokia.com wrote: > > > > > > > > > I'm surprised at these MAX string lengths. Looking at the RDS > > > > > standard it > > > > > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it > > > > > is > > > > > either 32 (2A group) or 64 (2B group). I don't know which group the > > > > > si4713 > > > > > uses. > > > > > > > > > > Can you clarify how this is used? > > > > > > > > > > Regards, > > > > > > > > > > Hans > > > > > > > > Well, PS_NAME can be 8 x n, but only 8 bytes are shown at once... > > > > so it keeps 'scrolling', or changes periodically. There's even > > > > commercial > > > > radio stations that do so. > > > > > > And I'm assuming that the same is true for radio text. However, this > > > behavior > > > contradicts the control description in the spec, so that should be > > > clarified. > > > > Yes, I'll add a comment explaining this for those defines. > > Another question: what happens if I give a string that's e.g. 10 characters > long? What will happen then? I believe receiver will still scroll, but may get confused. This case, it is better to pad with spaces. > > If the string must be exactly 8 x n long, then I think that it is a good idea > to start using the 'step' value of v4l2_queryctrl: this can be used to tell > the application that string lengths should be a multiple of the step value. > I've toyed with that idea before but I couldn't think of a good use case, > but this might be it. I think that would be good. It is a way to report to user land what can be done in these cases which strings can be chopped in small pieces. Of course, documenting this part it is appreciated. > > Regards, > > Hans > > -- > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- Eduardo Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Saturday 25 July 2009 15:25:21 Eduardo Valentin wrote: > On Sat, Jul 25, 2009 at 03:33:55PM +0200, ext Hans Verkuil wrote: > > On Saturday 25 July 2009 15:29:38 ext-eero.nurkk...@nokia.com wrote: > > > > > > > I'm surprised at these MAX string lengths. Looking at the RDS standard > > > > it > > > > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is > > > > either 32 (2A group) or 64 (2B group). I don't know which group the > > > > si4713 > > > > uses. > > > > > > > > Can you clarify how this is used? > > > > > > > > Regards, > > > > > > > > Hans > > > > > > Well, PS_NAME can be 8 x n, but only 8 bytes are shown at once... > > > so it keeps 'scrolling', or changes periodically. There's even commercial > > > radio stations that do so. > > > > And I'm assuming that the same is true for radio text. However, this > > behavior > > contradicts the control description in the spec, so that should be > > clarified. > > Yes, I'll add a comment explaining this for those defines. Another question: what happens if I give a string that's e.g. 10 characters long? What will happen then? If the string must be exactly 8 x n long, then I think that it is a good idea to start using the 'step' value of v4l2_queryctrl: this can be used to tell the application that string lengths should be a multiple of the step value. I've toyed with that idea before but I couldn't think of a good use case, but this might be it. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Sat, Jul 25, 2009 at 03:33:55PM +0200, ext Hans Verkuil wrote: > On Saturday 25 July 2009 15:29:38 ext-eero.nurkk...@nokia.com wrote: > > > > > I'm surprised at these MAX string lengths. Looking at the RDS standard it > > > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is > > > either 32 (2A group) or 64 (2B group). I don't know which group the si4713 > > > uses. > > > > > > Can you clarify how this is used? > > > > > > Regards, > > > > > > Hans > > > > Well, PS_NAME can be 8 x n, but only 8 bytes are shown at once... > > so it keeps 'scrolling', or changes periodically. There's even commercial > > radio stations that do so. > > And I'm assuming that the same is true for radio text. However, this behavior > contradicts the control description in the spec, so that should be clarified. Yes, I'll add a comment explaining this for those defines. > > Regards, > > Hans > > -- > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- Eduardo Valentin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Saturday 25 July 2009 15:29:38 ext-eero.nurkk...@nokia.com wrote: > > > I'm surprised at these MAX string lengths. Looking at the RDS standard it > > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is > > either 32 (2A group) or 64 (2B group). I don't know which group the si4713 > > uses. > > > > Can you clarify how this is used? > > > > Regards, > > > > Hans > > Well, PS_NAME can be 8 x n, but only 8 bytes are shown at once... > so it keeps 'scrolling', or changes periodically. There's even commercial > radio stations that do so. And I'm assuming that the same is true for radio text. However, this behavior contradicts the control description in the spec, so that should be clarified. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Sat, Jul 25, 2009 at 03:31:29PM +0200, ext Hans Verkuil wrote:
> On Saturday 25 July 2009 15:15:24 Eduardo Valentin wrote:
> > On Sat, Jul 25, 2009 at 03:20:53PM +0200, ext Hans Verkuil wrote:
> > > > + switch (control->id) {
> > > > + case V4L2_CID_RDS_TX_PS_NAME:
> > > > + if (strlen(sdev->rds_info.ps_name) + 1 > control->length)
> > > > {
> > > > + control->length = strlen(sdev->rds_info.ps_name)
> > > > + 1;
> > >
> > > I recommend setting length to the actual maximum MAX_RDS_PS_NAME+1.
> > >
> > > > + rval = -ENOSPC;
> > > > + goto exit;
> > > > + }
> > > > + rval = copy_to_user(control->string,
> > > > sdev->rds_info.ps_name,
> > > > + strlen(sdev->rds_info.ps_name) +
> > > > 1);
> > > > + break;
> > > > +
> > > > + case V4L2_CID_RDS_TX_RADIO_TEXT:
> > > > + if (strlen(sdev->rds_info.radio_text) + 1 >
> > > > control->length) {
> > > > + control->length =
> > > > strlen(sdev->rds_info.radio_text) + 1;
> > >
> > > Ditto.
> >
> > Right, I think doing the way you are proposing is to avoid changes that may
> > generate
> > failures in the following reads.
> >
> > I 'll change this in the v11 as well.
>
> OK.
>
> > > > +struct rds_info {
> > > > + u32 pi;
> > > > +#define MAX_RDS_PTY 31
> > > > + u32 pty;
> > > > +#define MAX_RDS_DEVIATION9
> > > > + u32 deviation;
> > > > +#define MAX_RDS_PS_NAME 96
> > > > + u8 ps_name[MAX_RDS_PS_NAME + 1];
> > > > +#define MAX_RDS_RADIO_TEXT 384
> > >
> > > I'm surprised at these MAX string lengths. Looking at the RDS standard it
> > > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is
> > > either 32 (2A group) or 64 (2B group). I don't know which group the si4713
> > > uses.
> > >
> > > Can you clarify how this is used?
>
> Did you see this comment as well? I'm quite interested in this.
I missed this one. But is basically what Eero said. Receivers scroll it with 8xn
sized PS names.
>
> Hans
>
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
Eduardo Valentin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
On Saturday 25 July 2009 15:15:24 Eduardo Valentin wrote:
> On Sat, Jul 25, 2009 at 03:20:53PM +0200, ext Hans Verkuil wrote:
> > > + switch (control->id) {
> > > + case V4L2_CID_RDS_TX_PS_NAME:
> > > + if (strlen(sdev->rds_info.ps_name) + 1 > control->length) {
> > > + control->length = strlen(sdev->rds_info.ps_name) +
> > > 1;
> >
> > I recommend setting length to the actual maximum MAX_RDS_PS_NAME+1.
> >
> > > + rval = -ENOSPC;
> > > + goto exit;
> > > + }
> > > + rval = copy_to_user(control->string, sdev->rds_info.ps_name,
> > > + strlen(sdev->rds_info.ps_name) + 1);
> > > + break;
> > > +
> > > + case V4L2_CID_RDS_TX_RADIO_TEXT:
> > > + if (strlen(sdev->rds_info.radio_text) + 1 >
> > > control->length) {
> > > + control->length = strlen(sdev->rds_info.radio_text)
> > > + 1;
> >
> > Ditto.
>
> Right, I think doing the way you are proposing is to avoid changes that may
> generate
> failures in the following reads.
>
> I 'll change this in the v11 as well.
OK.
> > > +struct rds_info {
> > > + u32 pi;
> > > +#define MAX_RDS_PTY 31
> > > + u32 pty;
> > > +#define MAX_RDS_DEVIATION9
> > > + u32 deviation;
> > > +#define MAX_RDS_PS_NAME 96
> > > + u8 ps_name[MAX_RDS_PS_NAME + 1];
> > > +#define MAX_RDS_RADIO_TEXT 384
> >
> > I'm surprised at these MAX string lengths. Looking at the RDS standard it
> > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is
> > either 32 (2A group) or 64 (2B group). I don't know which group the si4713
> > uses.
> >
> > Can you clarify how this is used?
Did you see this comment as well? I'm quite interested in this.
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv10 6/8] FMTx: si4713: Add files to handle si4713 i2c device
> I'm surprised at these MAX string lengths. Looking at the RDS standard it > seems that the max length for the PS_NAME is 8 and for RADIO_TEXT it is > either 32 (2A group) or 64 (2B group). I don't know which group the si4713 > uses. > > Can you clarify how this is used? > > Regards, > > Hans Well, PS_NAME can be 8 x n, but only 8 bytes are shown at once... so it keeps 'scrolling', or changes periodically. There's even commercial radio stations that do so. - Eero-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
