This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.
Álvaro Neira Ayuso alvaroneay at gmail.com
Hello Peter
El 15/03/14 02:45, Peter Stuge escribió:
> Alvaro Neira Ayuso wrote:
>> +++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.c
>> @@ -120,6 +120,12 @@ static struct msgb *sbts2050_ucinfo_get(struct uc *ucontrol, struct ucinfo info)
>> case SBTS2050_TEMP_RQT:
>> num = sizeof(command->cmd.tempGet);
>> break;
>> + case SBTS2050_PWR_RQT:
>> + num = sizeof(command->cmd.pwrSetState);
>> + break;
>
> A small style remark is that I like to use: sizeof command->cmd.pwerSetState
> since sizeof isn't really a function. Similar to return.
I have followed the kernel coding style, and he said doesn't speak about
use sizeof without bracket.
>
>
>> @@ -138,6 +144,17 @@ static struct msgb *sbts2050_ucinfo_get(struct uc *ucontrol, struct ucinfo info)
>> command->u8Id = info.id;
>> command->u8Len = sizeof(command->cmd.tempGet);
>> break;
>> + case SBTS2050_PWR_RQT:
>> + command->u8Id = info.id;
>> + command->u8Len = sizeof(command->cmd.pwrSetState);
>> + command->cmd.pwrSetState.u1MasterEn = !!info.master;
>> + command->cmd.pwrSetState.u1SlaveEn = !!info.slave;
>> + command->cmd.pwrSetState.u1PwrAmpEn = !!info.pa;
>> + break;
>> + case SBTS2050_PWR_STATUS:
>> + command->u8Id = info.id;
>> + command->u8Len = sizeof(command->cmd.pwrGetStatus);
>
> The above lines have a couple of extra spaces before the = which may
> or may not be worth fixing.
Yes, it's true, I don't know why I have done that...
>
>
>> +int sbts2050_uc_status(struct uc *ucontrol, enum sbts2050_status_rqt status)
> ..
>> + msg = sbts2050_ucinfo_get(ucontrol, info);
>> +
>> + if (msg == NULL) {
>> + LOGP(DTEMP, LOGL_ERROR, "Error switching off some unit");
>
> This error message doesn't contain very much information. It might be
> nice to know both which unit could not be switched off *and* what the
> error was.
>
> Also, is DTEMP guaranteed to always be the appropriate subsystem for
> this function? Maybe yes, but this code is in sysmobts_misc.c which
> suggests that it may be something more general?
I have used DTEMP, because i'm going to have a relation between the
temperature and this function but it's true that somebody can use it in
other cases. I don't know what do you think about that, if somebody can
help me.
>
>
>> +++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.h
>> @@ -13,6 +13,12 @@ enum sysmobts_temp_type {
>> _NUM_TEMP_TYPES
>> };
>>
>> +enum sbts2050_status_rqt {
>> + SBTS2050_STATUS_MASTER,
>> + SBTS2050_STATUS_SLAVE,
>> + SBTS2050_STATUS_PA
>
> Maybe add a , after the last enum to make it easier to add more
> values in the future.
>
>
> //Peter
>
Thanks a lot Peter