| Message ID | CA+ChNyXo96QfgoXd9PPSL0jeP+kNO+r4_Myepgsq3q9+Jm0i=A@mail.gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Thanks, this works for me, but I still don't understand why, the ordering of bitfields looks quite strange to me :) Jaroslav ----- Original Message ----- > Hi, > > In the end, I ended up with the following patch that works: > > diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h > b/include/osmocom/gsm/protocol/gsm_03_41.h > index 0ece6cc..40051cd 100644 > --- a/include/osmocom/gsm/protocol/gsm_03_41.h > +++ b/include/osmocom/gsm/protocol/gsm_03_41.h > @@ -2,8 +2,13 @@ > > #include <stdint.h> > > +#include <osmocom/core/endian.h> > #include <osmocom/gsm/protocol/gsm_04_12.h> > > +#ifndef OSMO_IS_LITTLE_ENDIAN > + #define OSMO_IS_LITTLE_ENDIAN 0 > +#endif > + > /* GSM TS 03.41 definitions also TS 23.041*/ > > #define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct gsm341_ms_message)) > @@ -13,19 +18,36 @@ > /* Chapter 9.3.2 */ > struct gsm341_ms_message { > struct { > +#if OSMO_IS_LITTLE_ENDIAN == 1 > uint8_t code_hi:6; > uint8_t gs:2; > uint8_t update:4; > uint8_t code_lo:4; > +#else > + uint8_t gs:2; > + uint8_t code_hi:6; > + uint8_t code_lo:4; > + uint8_t update:4; > +#endif > } serial; > uint16_t msg_id; > struct { > +#if OSMO_IS_LITTLE_ENDIAN == 1 > uint8_t language:4; > uint8_t group:4; > +#else > + uint8_t group:4; > + uint8_t language:4; > +#endif > } dcs; > struct { > +#if OSMO_IS_LITTLE_ENDIAN == 1 > uint8_t total:4; > uint8_t current:4; > +#else > + uint8_t current:4; > + uint8_t total:4; > +#endif > } page; > uint8_t data[0]; > } __attribute__((packed)); > @@ -33,12 +55,21 @@ struct gsm341_ms_message { > /* Chapter 9.4.1.3 */ > struct gsm341_etws_message { > struct { > +#if OSMO_IS_LITTLE_ENDIAN == 1 > uint8_t code_hi:4; > uint8_t popup:1; > uint8_t alert:1; > uint8_t gs:2; > uint8_t update:4; > uint8_t code_lo:4; > +#else > + uint8_t gs:2; > + uint8_t alert:1; > + uint8_t popup:1; > + uint8_t code_hi:4; > + uint8_t code_lo:4; > + uint8_t update:4; > +#endif > } serial; > uint16_t msg_id; > uint16_t warning_type; > > > Note that the order of the fields is a bit different than your proposal. > > I also, interestingly enough, found this: > http://lists.osmocom.org/pipermail/baseband-devel/2015-February/000021.html > > It's from Februrary, and already a suggestion for a patch for this > problem. I don't think however that patch > will work with other big-endian architectures. > > Cheers, > Ruben > > > 2015-12-07 22:16 GMT+01:00 Ruben Undheim <ruben.undheim@gmail.com>: > > Hi, > > > > Thanks! > > > > Apparently it doesn't work correctly, but I'm now trying with: > > #if OSMO_IS_LITTLE_ENDIAN == 1 > > > > instead, and I have big hopes. > > > > (OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 > > for big-endian archs.) > > > > Cheers, > > Ruben > > > > 2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>: > >> Hi Ruben, > >> > >> On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote: > >>> While building the package for Debian, apparently there is a problem > >>> related to big-endian architectures. > >> > >> While I still own several PPC machines, I haven't booted any of them in > >> years, and don't have a build setup ready. Please try the patch > >> below and report back if it works. If yes, we can merge it. > >> > >> From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 > >> From: Harald Welte <laforge@gnumonks.org> > >> Date: Sun, 6 Dec 2015 22:12:43 +0100 > >> Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines > >> > >> Our gsm_03_41 structs use bit-fields, but don't do the usual > >> little/big-endian jumping. > >> --- > >> include/osmocom/gsm/protocol/gsm_03_41.h | 27 +++++++++++++++++++++++++++ > >> 1 file changed, 27 insertions(+) > >> > >> diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h > >> b/include/osmocom/gsm/protocol/gsm_03_41.h > >> index 0ece6cc..f007cc1 100644 > >> --- a/include/osmocom/gsm/protocol/gsm_03_41.h > >> +++ b/include/osmocom/gsm/protocol/gsm_03_41.h > >> @@ -2,6 +2,7 @@ > >> > >> #include <stdint.h> > >> > >> +#include <osmocom/core/endian.h> > >> #include <osmocom/gsm/protocol/gsm_04_12.h> > >> > >> /* GSM TS 03.41 definitions also TS 23.041*/ > >> @@ -13,19 +14,36 @@ > >> /* Chapter 9.3.2 */ > >> struct gsm341_ms_message { > >> struct { > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > >> uint8_t code_hi:6; > >> uint8_t gs:2; > >> uint8_t update:4; > >> uint8_t code_lo:4; > >> +#else > >> + uint8_t code_lo:4; > >> + uint8_t update:4; > >> + uint8_t gs:2; > >> + uint8_t code_hi:6; > >> +#endif > >> } serial; > >> uint16_t msg_id; > >> struct { > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > >> uint8_t language:4; > >> uint8_t group:4; > >> +#else > >> + uint8_t group:4; > >> + uint8_t language:4; > >> +#endif > >> } dcs; > >> struct { > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > >> uint8_t total:4; > >> uint8_t current:4; > >> +#else > >> + uint8_t current:4; > >> + uint8_t total:4; > >> +#endif > >> } page; > >> uint8_t data[0]; > >> } __attribute__((packed)); > >> @@ -33,12 +51,21 @@ struct gsm341_ms_message { > >> /* Chapter 9.4.1.3 */ > >> struct gsm341_etws_message { > >> struct { > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > >> uint8_t code_hi:4; > >> uint8_t popup:1; > >> uint8_t alert:1; > >> uint8_t gs:2; > >> uint8_t update:4; > >> uint8_t code_lo:4; > >> +#else > >> + uint8_t code_lo:4; > >> + uint8_t update:4; > >> + uint8_t gs:2; > >> + uint8_t alert:1; > >> + uint8_t popup:1; > >> + uint8_t code_hi:4; > >> +#endif > >> } serial; > >> uint16_t msg_id; > >> uint16_t warning_type; > >> -- > >> 2.6.2 > >> > >> -- > >> - Harald Welte <laforge@gnumonks.org> > >> http://laforge.gnumonks.org/ > >> ============================================================================ > >> "Privacy in residential applications is a desirable marketing option." > >> (ETSI EN 300 175-7 Ch. > >> A6) >
I am afraid using bitfields is not portable and bits ordering may differ between architectures and compilers (it's not only about endianess, IMHO it's not defined by the standard). I think correct portable implementation should use macros and bitmasks, maybe something to consider for future updates thanks & regards Jaroslav ----- Original Message ----- > Thanks, > > this works for me, but I still don't understand why, the ordering > of bitfields looks quite strange to me :) > > Jaroslav > > ----- Original Message ----- > > Hi, > > > > In the end, I ended up with the following patch that works: > > > > diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h > > b/include/osmocom/gsm/protocol/gsm_03_41.h > > index 0ece6cc..40051cd 100644 > > --- a/include/osmocom/gsm/protocol/gsm_03_41.h > > +++ b/include/osmocom/gsm/protocol/gsm_03_41.h > > @@ -2,8 +2,13 @@ > > > > #include <stdint.h> > > > > +#include <osmocom/core/endian.h> > > #include <osmocom/gsm/protocol/gsm_04_12.h> > > > > +#ifndef OSMO_IS_LITTLE_ENDIAN > > + #define OSMO_IS_LITTLE_ENDIAN 0 > > +#endif > > + > > /* GSM TS 03.41 definitions also TS 23.041*/ > > > > #define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct > > gsm341_ms_message)) > > @@ -13,19 +18,36 @@ > > /* Chapter 9.3.2 */ > > struct gsm341_ms_message { > > struct { > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t code_hi:6; > > uint8_t gs:2; > > uint8_t update:4; > > uint8_t code_lo:4; > > +#else > > + uint8_t gs:2; > > + uint8_t code_hi:6; > > + uint8_t code_lo:4; > > + uint8_t update:4; > > +#endif > > } serial; > > uint16_t msg_id; > > struct { > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t language:4; > > uint8_t group:4; > > +#else > > + uint8_t group:4; > > + uint8_t language:4; > > +#endif > > } dcs; > > struct { > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t total:4; > > uint8_t current:4; > > +#else > > + uint8_t current:4; > > + uint8_t total:4; > > +#endif > > } page; > > uint8_t data[0]; > > } __attribute__((packed)); > > @@ -33,12 +55,21 @@ struct gsm341_ms_message { > > /* Chapter 9.4.1.3 */ > > struct gsm341_etws_message { > > struct { > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t code_hi:4; > > uint8_t popup:1; > > uint8_t alert:1; > > uint8_t gs:2; > > uint8_t update:4; > > uint8_t code_lo:4; > > +#else > > + uint8_t gs:2; > > + uint8_t alert:1; > > + uint8_t popup:1; > > + uint8_t code_hi:4; > > + uint8_t code_lo:4; > > + uint8_t update:4; > > +#endif > > } serial; > > uint16_t msg_id; > > uint16_t warning_type; > > > > > > Note that the order of the fields is a bit different than your proposal. > > > > I also, interestingly enough, found this: > > http://lists.osmocom.org/pipermail/baseband-devel/2015-February/000021.html > > > > It's from Februrary, and already a suggestion for a patch for this > > problem. I don't think however that patch > > will work with other big-endian architectures. > > > > Cheers, > > Ruben > > > > > > 2015-12-07 22:16 GMT+01:00 Ruben Undheim <ruben.undheim@gmail.com>: > > > Hi, > > > > > > Thanks! > > > > > > Apparently it doesn't work correctly, but I'm now trying with: > > > #if OSMO_IS_LITTLE_ENDIAN == 1 > > > > > > instead, and I have big hopes. > > > > > > (OSMO_IS_LITTLE_ENDIAN is always defined, 1 for low-endian archs and 0 > > > for big-endian archs.) > > > > > > Cheers, > > > Ruben > > > > > > 2015-12-06 22:15 GMT+01:00 Harald Welte <laforge@gnumonks.org>: > > >> Hi Ruben, > > >> > > >> On Sun, Dec 06, 2015 at 04:53:09PM +0100, Ruben Undheim wrote: > > >>> While building the package for Debian, apparently there is a problem > > >>> related to big-endian architectures. > > >> > > >> While I still own several PPC machines, I haven't booted any of them in > > >> years, and don't have a build setup ready. Please try the patch > > >> below and report back if it works. If yes, we can merge it. > > >> > > >> From 51ae645e220556bbeabce3ac57304639328e2164 Mon Sep 17 00:00:00 2001 > > >> From: Harald Welte <laforge@gnumonks.org> > > >> Date: Sun, 6 Dec 2015 22:12:43 +0100 > > >> Subject: [PATCH] untested fix for gsm_03_41.h and big-endian machines > > >> > > >> Our gsm_03_41 structs use bit-fields, but don't do the usual > > >> little/big-endian jumping. > > >> --- > > >> include/osmocom/gsm/protocol/gsm_03_41.h | 27 > > >> +++++++++++++++++++++++++++ > > >> 1 file changed, 27 insertions(+) > > >> > > >> diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h > > >> b/include/osmocom/gsm/protocol/gsm_03_41.h > > >> index 0ece6cc..f007cc1 100644 > > >> --- a/include/osmocom/gsm/protocol/gsm_03_41.h > > >> +++ b/include/osmocom/gsm/protocol/gsm_03_41.h > > >> @@ -2,6 +2,7 @@ > > >> > > >> #include <stdint.h> > > >> > > >> +#include <osmocom/core/endian.h> > > >> #include <osmocom/gsm/protocol/gsm_04_12.h> > > >> > > >> /* GSM TS 03.41 definitions also TS 23.041*/ > > >> @@ -13,19 +14,36 @@ > > >> /* Chapter 9.3.2 */ > > >> struct gsm341_ms_message { > > >> struct { > > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > > >> uint8_t code_hi:6; > > >> uint8_t gs:2; > > >> uint8_t update:4; > > >> uint8_t code_lo:4; > > >> +#else > > >> + uint8_t code_lo:4; > > >> + uint8_t update:4; > > >> + uint8_t gs:2; > > >> + uint8_t code_hi:6; > > >> +#endif > > >> } serial; > > >> uint16_t msg_id; > > >> struct { > > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > > >> uint8_t language:4; > > >> uint8_t group:4; > > >> +#else > > >> + uint8_t group:4; > > >> + uint8_t language:4; > > >> +#endif > > >> } dcs; > > >> struct { > > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > > >> uint8_t total:4; > > >> uint8_t current:4; > > >> +#else > > >> + uint8_t current:4; > > >> + uint8_t total:4; > > >> +#endif > > >> } page; > > >> uint8_t data[0]; > > >> } __attribute__((packed)); > > >> @@ -33,12 +51,21 @@ struct gsm341_ms_message { > > >> /* Chapter 9.4.1.3 */ > > >> struct gsm341_etws_message { > > >> struct { > > >> +#ifdef OSMO_IS_LITTLE_ENDIAN > > >> uint8_t code_hi:4; > > >> uint8_t popup:1; > > >> uint8_t alert:1; > > >> uint8_t gs:2; > > >> uint8_t update:4; > > >> uint8_t code_lo:4; > > >> +#else > > >> + uint8_t code_lo:4; > > >> + uint8_t update:4; > > >> + uint8_t gs:2; > > >> + uint8_t alert:1; > > >> + uint8_t popup:1; > > >> + uint8_t code_hi:4; > > >> +#endif > > >> } serial; > > >> uint16_t msg_id; > > >> uint16_t warning_type; > > >> -- > > >> 2.6.2 > > >> > > >> -- > > >> - Harald Welte <laforge@gnumonks.org> > > >> http://laforge.gnumonks.org/ > > >> ============================================================================ > > >> "Privacy in residential applications is a desirable marketing option." > > >> (ETSI EN 300 175-7 Ch. > > >> A6) > > >
Hi Ruben,
On Wed, Dec 09, 2015 at 06:20:58PM +0100, Ruben Undheim wrote:
> In the end, I ended up with the following patch that works:
Thansk. Your e-mail client mangled the whitespaces/tabs, but I fixed it
up manually. Now pushed to master.
Regards,
Harald
On Wed, Dec 09, 2015 at 12:41:33PM -0500, Jaroslav Skarvada wrote: > this works for me, but I still don't understand why, the ordering > of bitfields looks quite strange to me :) The reversal needs to be done byte-wise. So in this example: > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t code_hi:6; > > uint8_t gs:2; > > uint8_t update:4; > > uint8_t code_lo:4; The first byte is code_hi and gs, reverse those: > > +#else > > + uint8_t gs:2; > > + uint8_t code_hi:6; Next byte is update and code_lo, reversed: > > + uint8_t code_lo:4; > > + uint8_t update:4; > > +#endif I hope that helps :) ~Neels > > +#if OSMO_IS_LITTLE_ENDIAN == 1 > > uint8_t code_hi:4; > > uint8_t popup:1; > > uint8_t alert:1; > > uint8_t gs:2; > > uint8_t update:4; > > uint8_t code_lo:4; > > +#else > > + uint8_t gs:2; > > + uint8_t alert:1; > > + uint8_t popup:1; > > + uint8_t code_hi:4; > > + uint8_t code_lo:4; > > + uint8_t update:4; > > +#endif
On Wed, Dec 09, 2015 at 01:26:41PM -0500, Jaroslav Skarvada wrote: > I think correct portable implementation should use macros and > bitmasks, maybe something to consider for future updates IP headers and TCP headers are always defined as bit filds, look at the netinet/ip.h and netinet/tcp.h. We're in the best of traditions here ;)
> Your e-mail client mangled the whitespaces/tabs, but I fixed it > up manually. Sorry about that. Next time I won't use the gmail web client for that kind of stuff. :) Thanks to Neels for making things clear ! Cheers, Ruben
On Thu, Dec 10, 2015 at 09:51:14AM +0100, Harald Welte wrote: > On Wed, Dec 09, 2015 at 01:26:41PM -0500, Jaroslav Skarvada wrote: > > I think correct portable implementation should use macros and > > bitmasks, maybe something to consider for future updates > > IP headers and TCP headers are always defined as bit filds, look at the > netinet/ip.h and netinet/tcp.h. We're in the best of traditions here ;) ...not that it would win a code beauty contest though :) Maybe a consolation prize for the most widely used redundancy... ~Neels
> On 09 Dec 2015, at 18:20, Ruben Undheim <ruben.undheim@gmail.com> wrote: > > Hi, > > > +#ifndef OSMO_IS_LITTLE_ENDIAN > + #define OSMO_IS_LITTLE_ENDIAN 0 > +#endif I am surprised by this hunk. Our endian.h should define both macros. I think I used the vim search to search for a typo in one of the branches (BSD vs. Linux, LE vs. BE) but it looks good. Do you remember why this was needed? holger
diff --git a/include/osmocom/gsm/protocol/gsm_03_41.h b/include/osmocom/gsm/protocol/gsm_03_41.h index 0ece6cc..40051cd 100644 --- a/include/osmocom/gsm/protocol/gsm_03_41.h +++ b/include/osmocom/gsm/protocol/gsm_03_41.h @@ -2,8 +2,13 @@ #include <stdint.h> +#include <osmocom/core/endian.h> #include <osmocom/gsm/protocol/gsm_04_12.h> +#ifndef OSMO_IS_LITTLE_ENDIAN + #define OSMO_IS_LITTLE_ENDIAN 0 +#endif + /* GSM TS 03.41 definitions also TS 23.041*/ #define GSM341_MAX_PAYLOAD (GSM412_MSG_LEN-sizeof(struct gsm341_ms_message)) @@ -13,19 +18,36 @@ /* Chapter 9.3.2 */ struct gsm341_ms_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:6; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t code_hi:6; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t language:4; uint8_t group:4; +#else + uint8_t group:4; + uint8_t language:4; +#endif } dcs; struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t total:4; uint8_t current:4; +#else + uint8_t current:4; + uint8_t total:4; +#endif } page; uint8_t data[0]; } __attribute__((packed)); @@ -33,12 +55,21 @@ struct gsm341_ms_message { /* Chapter 9.4.1.3 */ struct gsm341_etws_message { struct { +#if OSMO_IS_LITTLE_ENDIAN == 1 uint8_t code_hi:4; uint8_t popup:1; uint8_t alert:1; uint8_t gs:2; uint8_t update:4; uint8_t code_lo:4; +#else + uint8_t gs:2; + uint8_t alert:1; + uint8_t popup:1; + uint8_t code_hi:4; + uint8_t code_lo:4; + uint8_t update:4; +#endif } serial; uint16_t msg_id; uint16_t warning_type;