On Sat, 2021-01-02 at 18:20 +0100, Helge Kruse wrote:
>
> I use the Kingst LA2016 logic analyser and found that it doesn't work in
> Windows environment. There is an USB transfer that is built from a
> packed struct:
>
> typedef struct sample_config {
> uint32_t sample_depth;
> uint32_t psa;
> uint16_t u1;
> uint32_t u2;
> uint16_t clock_divisor;
> } __attribute__((__packed__)) sample_config_t;
>
> [ ... ]
>
> Linux:
> 40 0D 03 00 00 10 27 00 00 00 66 66 66 00 01 00
> Windows:
> 40 0D 03 00 00 10 27 00 00 00 00 00 66 66 66 00 01 00
There is the problem. Good that you tracked down its location.
> The MXE compiler, that is used for compiling the Windows version ignores
> silently this attribute. This can be verified by a simple "Hello, World"
> style program.
>
> I have implemented a workaround and pushed it to
> https://github.com/harper23/libsigrok. I am not satisfied with the
> approach, because the struct is first filled, but not used to actually
> transfer the data. Instead an additional buffer is used. [ ... ]
>
> I also thought about a conditional compilation but didn't found a macro
> to evaluate with the preprocessor. I think it's also not good to add
> such conditionals because of a compiler bug.
Sorry, but I cannot see how this is "a compiler bug". The number
of leading underscores in the application source code should make
the alarm bells go off, screamingly loud. The compiler is not at
fault here.
The initial problem is in the non-portable phrasing of the code.
All the research and hints in responses so far (several of them,
which is why I reply to your original message which has the
essential information) are" improving that bug". This may work
around one rather specific setup where the issue was observed
right now. But "a better workaround" still remains a workaround,
and should not be confused with a solution. The result of such a
"better attribute spec" is code which still is non-portable, and
will only work with some specific compiler of specific versions
and configurations, on those platforms where the specific
workaround is "supported". A proper resolution should work
equally well everywhere.
The "packed" attribute may have its use in some specific
situations. But regular application code in a project striving
for portability isn't one of them.
The proper thing to do is what you suggested. Use structs for
internal application purposes. Translate the data at boundaries
between the application and a communication channel, where one
specific memory layout is expected. Leave the application part as
transparent as it should be, don't "constrain" your application
with the details of a physical transport. BTW using packed
structs in the transport conversion isn't right either, it's as
non-portable as packed structs in application code are.
There is no need to re-invent the layout of bytes in a stream.
Common and tested support exists, the sigrok project has a set of
endianess aware conversion helpers for this very purpose, because
the situation isn't new at all. See read_u8() and its friends.
The _inc() variants are especially convenient for the situation
that you describe above.
Moving existing drivers that suffer from portability issues can
be considered low hanging fruit for those who want to help other
users, and unbreak the operation of existing drivers in
environments where they did not work before. Such a proper fix
would no doubt get accepted into mainline, since it's a clear
improvement, not just moving the problem to a different location.
See the asix-sigma and itech-it8500 drivers or the saleae input
module for examples how to read and write byte streams of a given
layout. Especially the WRITE_CLOCK_SELECT register setup and the
itech_it8500_get_status() packet interpretation demonstrate mixed
data types. The uni-t-ut181a driver was written before that, and
uses a mix of DIY and "old" WL16() et al code. Somebody should
talk to that driver's author. :-P I invite everybody who wants
to express support for the sigrok project to search for low
hanging fruit, and help improve the code base. Kudos to tjko for
already having done that. :) The "old" WL16() etc macros BTW
were changed to use the new intrinsics, their implementation as
cpp macros suffered from robustness issues. Other bug fixes in
the past revealed that redundant local copies are not always
right, e.g. get signedness wrong. That's another reason to not
re-invent this stuff in calling code. The use of common helpers
transparently benefits all code that existed before any future
improvement, should an issue get identified.
$ grep -r read_u src/hardware/ src/input
$ grep -r write_u src/hardware/ src/input
$ grep -r [WR]L src/hardware/uni-t-ut181a
virtually yours
Gerhard Sittig
--
If you don't understand or are scared by any of the above
ask your parents or an adult to help you.
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
