gh-raspberrypi/pico-sdk
Watch
1
0
Fork
You've already forked pico-sdk
 0
Code Issues 262 Pull requests 63 Releases Activity

Swap endian-ness in tools on big endian hosts #329

Open
zeldin wants to merge 3 commits from zeldin/endianness into develop
pull from: zeldin/endianness
merge into: gh-raspberrypi:develop
Conversation 24 Commits 3 Files changed 7 +122 -64
zeldin commented 2021-04-10 06:25:38 -05:00 (Migrated from github.com)

When the host running the SDK is not little endian, all data that is exchanged with the Pico must be converted to/from little endian byte order.

Instead of sprinking explicit swaps around the code, I used available mechanisms for doing the swaps implicitly. Unfortunately, there is no such mechanism that works with both C and C++, so because the code base mixes C and C++ double mechanisms need to be employed...

The C++ fix also suffers from the fact that some structures are declared as __attribute__((packed)), which requires the contents to be POD. Because of this, the little_endian template is not allowed to have a constructor. This is not a problem in the SDK itself, but in picotool it would have been convenient to be able to create std::vector<little_endian<uint32_t>> program with an initializer.

When the host running the SDK is not little endian, all data that is exchanged with the Pico must be converted to/from little endian byte order. Instead of sprinking explicit swaps around the code, I used available mechanisms for doing the swaps implicitly. Unfortunately, there is no such mechanism that works with both C and C++, so because the code base mixes C and C++ double mechanisms need to be employed... The C++ fix also suffers from the fact that some structures are declared as `__attribute__((packed))`, which requires the contents to be POD. Because of this, the `little_endian` template is not allowed to have a constructor. This is not a problem in the SDK itself, but in `picotool` it would have been convenient to be able to create `std::vector<little_endian<uint32_t>> program` with an initializer.
lurch commented 2021-04-10 06:59:23 -05:00 (Migrated from github.com)

@kilograham may feel differently (and his opinion matters much more than mine), but I think it might be simpler to keep everything on the Pico little-endian, and rely on the host-side tools (elf2uf2, picotool, etc.) to do any necessary conversion if the host itself isn't little-endian?
Otherwise you might have some Picos presenting a little-endian host interface, and other Picos presenting a big-endian host interface, depending only on what firmware has been flashed to them, and that might get a bit confusing?

(Of course, that's assuming that I'm correctly understanding what this PR is trying to do, which I might not be? 🤷 )

@kilograham may feel differently (and his opinion matters much more than mine), but I think it might be simpler to keep everything on the Pico little-endian, and rely on the host-side tools (`elf2uf2`, `picotool`, etc.) to do any necessary conversion if the host itself isn't little-endian? Otherwise you might have some Picos presenting a little-endian host interface, and other Picos presenting a big-endian host interface, depending only on what firmware has been flashed to them, and that might get a bit confusing? (Of course, that's assuming that I'm correctly understanding what this PR is trying to do, which I might not be? :shrug: )
zeldin commented 2021-04-10 07:05:00 -05:00 (Migrated from github.com)

@lurch I think there may be a misunderstanding here. What the code does it make the data being accessed as little endian, regardless of the systems actual endianness. On a system which is already little endian, such as the Pico itself, this is a no-operation (and in fact the macros will expand to nothing). There is nothing here that will make anything big endian on the Pico. All conversions happen on the host side (and only if the host is big endian).

@lurch I think there may be a misunderstanding here. What the code does it make the data being accessed as little endian, regardless of the systems actual endianness. On a system which is already little endian, such as the Pico itself, this is a no-operation (and in fact the macros will expand to nothing). There is nothing here that will make anything big endian on the Pico. All conversions happen on the host side (and only if the host is big endian).
👍 1
lurch commented 2021-04-10 07:16:20 -05:00 (Migrated from github.com)

All conversions happen on the host side (and only if the host is big endian).

Cool, please accept my apologies for my misunderstanding (I'm more of a Python guy than a low-level C guy). Is a similar patch needed for https://github.com/raspberrypi/picotool or does this PR already encompass that?

> All conversions happen on the host side (and only if the host is big endian). Cool, please accept my apologies for my misunderstanding (I'm more of a Python guy than a low-level C guy). Is a similar patch needed for https://github.com/raspberrypi/picotool or does this PR already encompass that?
zeldin commented 2021-04-10 07:18:45 -05:00 (Migrated from github.com)

@lurch Yes, I have a patch for picotool too. It depends on this patch though, so I wanted to test the waters here first before submitting it. 😄

@lurch Yes, I have a patch for `picotool` too. It depends on this patch though, so I wanted to test the waters here first before submitting it. :smile:
😆 1
lurch commented 2021-04-10 07:21:44 -05:00 (Migrated from github.com)

Just out of curiosity, what big-endian host machine / OS / environment are you using?

Just out of curiosity, what big-endian host machine / OS / environment are you using?
zeldin commented 2021-04-10 07:25:02 -05:00 (Migrated from github.com)

@lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. 😸

@lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. :smile_cat:
👀 1 🚀 2
aallan commented 2021-04-10 07:53:50 -05:00 (Migrated from github.com)

@lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. 😸

I'd presumed you were on a Dec Alpha or a VAX or some such. Huh. I hadn't quite grokked that the Pi was Bi-endian.

> @lurch RCS Talos II (POWER9) with Gentoo Linux is my main host. I also have a big endian Raspberry Pi 3. 😸 I'd presumed you were on a Dec Alpha or a VAX or some such. Huh. I hadn't quite grokked that the Pi was Bi-endian.
zeldin commented 2021-04-10 08:08:11 -05:00 (Migrated from github.com)

@aallan All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. 😆

@aallan All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. :laughing:
aallan commented 2021-04-10 08:15:09 -05:00 (Migrated from github.com)

All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. 😆

Having used a VAX a lot during my undergrad I have total false memories of it being big endian at that point. Huh. Guess I was brain washed by my many years on Dec Alphas during my postgrad years. 🤷‍♂️

> All ARM-v8 cores from ARM are bi-endian. VAX is little-endian only, though, having been designed in the 1970:s, just like x86. 😆 Having used a VAX a lot during my undergrad I have total false memories of it being big endian at that point. Huh. Guess I was brain washed by my many years on Dec Alphas during my postgrad years. 🤷‍♂️
kilograham commented 2021-04-15 15:36:27 -05:00 (Migrated from github.com)

can you rebase this on develop

can you rebase this on develop
zeldin commented 2021-04-16 11:58:27 -05:00 (Migrated from github.com)

@kilograham Sure thing!

@kilograham Sure thing!
kilograham commented 2021-04-18 18:00:28 -05:00 (Migrated from github.com)

I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode). I don't believe we have any non target device which touches this which isn't C++ anyway.

I'd be tempted perhaps to do (i will keep trying to think of a nice way to do this with typedefs - I guess we can guard with an #ifdef)

#ifndef le_uint32_t
#define le_uint32_t uint32_t
#endif

in the headers (or in a common one if we have one which works across elf2uf2, picotool, regular build etc)

And that way we defer everything to the user.

C code, can continue to have to reverse byte orders manually (or wrap the structure with another one which reverses the byte order using scalar_storage_order - if that's what they choose to do)

In our C++ we can define le_uint32_t to be stored_little_endian<uint32_t> before including the header

I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode). I don't believe we have any non target device which touches this which isn't C++ anyway. I'd be tempted perhaps to do (i will keep trying to think of a nice way to do this with typedefs - I guess we can guard with an #ifdef) ```c #ifndef le_uint32_t #define le_uint32_t uint32_t #endif ``` in the headers (or in a common one if we have one which works across elf2uf2, picotool, regular build etc) And that way we defer everything to the user. C code, can continue to have to reverse byte orders manually (or wrap the structure with another one which reverses the byte order using scalar_storage_order - if that's what they choose to do) In our C++ we can define `le_uint32_t` to be `stored_little_endian<uint32_t>` before including the header
zeldin commented 2021-04-19 15:41:22 -05:00 (Migrated from github.com)

I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode).

Ah, ok. In that case at the very least there should be a conditional #error to catch the case where you use clang on a big-endian host. But not using it at all might be preferable.

I don't believe we have any non target device which touches this which isn't C++ anyway.

Maybe I mistunderstand your point here, but C code compiled for the host using the relevant structs exist in the file picoboot_connection.c (which is in picotool, but the header file picoboot.h that it gets the structs from is in pico-sdk).
Of course, changing this file to C++ could be an option.

In our C++ we can define le_uint32_t to be stored_little_endian<uint32_t> before including the header

Right, but wouldn't it make sense for platform.h to make that define and provide the appropriate template? If you don't like e.g. uf2.h getting an added #include "pico/platform.h", the C++ source code could be required to include it from the top level instead. Or I guess a new header file with a different name. It just doesn't seem like this wheel should have to be reinvented by each client. 😸

> I'm a bit torn over this; scalar_storage_order isn't supported on clang (even gcc mode). Ah, ok. In that case at the very least there should be a conditional `#error` to catch the case where you use clang on a big-endian host. But not using it at all might be preferable. > I don't believe we have any non target device which touches this which isn't C++ anyway. Maybe I mistunderstand your point here, but C code compiled for the host using the relevant structs exist in the file `picoboot_connection.c` (which is in `picotool`, but the header file `picoboot.h` that it gets the structs from is in `pico-sdk`). Of course, changing this file to C++ could be an option. > In our C++ we can define `le_uint32_t` to be `stored_little_endian<uint32_t>` before including the header Right, but wouldn't it make sense for `platform.h` to make that define and provide the appropriate template? If you don't like e.g. `uf2.h` getting an added `#include "pico/platform.h"`, the C++ source code could be required to include it from the top level instead. Or I guess a new header file with a different name. It just doesn't seem like this wheel should have to be reinvented by each client. :smile_cat:
zeldin commented 2021-05-01 08:12:33 -05:00 (Migrated from github.com)

@kilograham I reworked the patch to work along the lines you described. The SDK patch wasn't affected that much. The picotool patch became a bit more messy when picoboot_connection.c can't rely on __attribute__((scalar_storage_order)) anymore, but it's not too bad. I'll open a PR for that repo to so you can take a look.

@kilograham I reworked the patch to work along the lines you described. The SDK patch wasn't affected that much. The `picotool` patch became a bit more messy when `picoboot_connection.c` can't rely on `__attribute__((scalar_storage_order))` anymore, but it's not too bad. I'll open a PR for that repo to so you can take a look.
kilograham (Migrated from github.com) reviewed 2021-05-01 14:48:28 -05:00
kilograham (Migrated from github.com) left a comment

a few thoughts; will look at the picotool patch too

a few thoughts; will look at the picotool patch too
src/common/pico_binary_info/include/pico/binary_info/structure.h
@ -21,6 +21,27 @@ extern "C" {
#define __packed __attribute__((packed))
#endif
#if !defined(le_uint16_t) || !defined(le_uint32_t) || !defined(le_int32_t)
kilograham (Migrated from github.com) commented 2021-05-01 14:45:11 -05:00

seems like this can go in platform.h, no?

seems like this can go in platform.h, no?
src/host/pico_platform/include/pico/platform.h
@ -135,5 +135,38 @@ static inline void __compiler_memory_barrier(void) {
}
#ifdef __cplusplus
}
kilograham (Migrated from github.com) commented 2021-05-01 14:47:46 -05:00

note we should only need this for big endian systems... either omit, or make it a no-op on little endian host

note we should only need this for big endian systems... either omit, or make it a no-op on little endian host
tools/elf2uf2/main.cpp
@ -9,7 +9,9 @@
#include <vector>
kilograham (Migrated from github.com) commented 2021-05-01 14:48:02 -05:00

this could be in platform.h

this could be in platform.h
zeldin (Migrated from github.com) reviewed 2021-05-01 15:22:05 -05:00
src/common/pico_binary_info/include/pico/binary_info/structure.h
@ -21,6 +21,27 @@ extern "C" {
#define __packed __attribute__((packed))
#endif
#if !defined(le_uint16_t) || !defined(le_uint32_t) || !defined(le_int32_t)
zeldin (Migrated from github.com) commented 2021-05-01 15:22:05 -05:00

Yes, but the thing is that structure.h does not include platform.h. Which is why it already has its own definition of __packed just above.

In the previous version of the patch, I did add an include of platform.h here, but then I noticed the comment at the start which says that this file is not supposed to use SDK includes...

Yes, but the thing is that `structure.h` does not include `platform.h`. Which is why it already has its own definition of `__packed` just above. In the previous version of the patch, I did add an include of `platform.h` here, but then I noticed the comment at the start which says that this file is not supposed to use SDK includes...
zeldin (Migrated from github.com) reviewed 2021-05-01 15:26:02 -05:00
tools/elf2uf2/main.cpp
@ -9,7 +9,9 @@
#include <vector>
zeldin (Migrated from github.com) commented 2021-05-01 15:26:02 -05:00

Ok, I guess if you want an alternate implementation you could make your #defines before including pico/platform.h instead of after.

Ok, I guess if you want an alternate implementation you could make your `#define`s before including `pico/platform.h` instead of after.
zeldin (Migrated from github.com) reviewed 2021-05-01 17:58:08 -05:00
src/host/pico_platform/include/pico/platform.h
@ -135,5 +135,38 @@ static inline void __compiler_memory_barrier(void) {
}
#ifdef __cplusplus
}
zeldin (Migrated from github.com) commented 2021-05-01 17:58:08 -05:00

Sure. A modern compiler will in fact already turn this code into a no-op on little endian, but I can put an explicitly no-op version inside an #ifdef.

Sure. A modern compiler will in fact already turn this code into a no-op on little endian, but I can put an explicitly no-op version inside an `#ifdef`.
zeldin (Migrated from github.com) reviewed 2021-05-02 08:09:58 -05:00
src/host/pico_platform/include/pico/platform.h
@ -135,5 +135,38 @@ static inline void __compiler_memory_barrier(void) {
}
#ifdef __cplusplus
}
zeldin (Migrated from github.com) commented 2021-05-02 08:09:58 -05:00

(Here's what I mean about the compiler fixing this for you, btw: https://godbolt.org/z/3cTdTYGzY)

(Here's what I mean about the compiler fixing this for you, btw: https://godbolt.org/z/3cTdTYGzY)
kilograham (Migrated from github.com) reviewed 2021-05-02 08:30:53 -05:00
src/common/pico_binary_info/include/pico/binary_info/structure.h
@ -21,6 +21,27 @@ extern "C" {
#define __packed __attribute__((packed))
#endif
#if !defined(le_uint16_t) || !defined(le_uint32_t) || !defined(le_int32_t)
kilograham (Migrated from github.com) commented 2021-05-02 08:30:53 -05:00

ah yes; danger of reading in github - yes we should keep that as you have it

ah yes; danger of reading in github - yes we should keep that as you have it
kilograham (Migrated from github.com) reviewed 2021-05-02 08:32:10 -05:00
src/host/pico_platform/include/pico/platform.h
@ -135,5 +135,38 @@ static inline void __compiler_memory_barrier(void) {
}
#ifdef __cplusplus
}
kilograham (Migrated from github.com) commented 2021-05-02 08:32:09 -05:00

Yeah, I figured, I didn't know they all did so so uniformly... still it is clearer, and some maniacs may use -O0.

Yeah, I figured, I didn't know they all did so so uniformly... still it is clearer, and some maniacs may use -O0.
kilograham commented 2021-06-01 11:12:46 -05:00 (Migrated from github.com)

Moved to 1.2.1 as I think i want to put the stuff in platform.h after all

Moved to 1.2.1 as I think i want to put the stuff in platform.h after all
josch commented 2024-06-05 04:51:05 -05:00 (Migrated from github.com)

We are now carrying this as a patch to the Debian package of pico-sdk so that it works on s390x:

https://salsa.debian.org/debian/pico-sdk/-/blob/master/debian/patches/329.patch?ref_type=heads

This closes raspberrypi/picotool#104

We are now carrying this as a patch to the Debian package of pico-sdk so that it works on s390x: https://salsa.debian.org/debian/pico-sdk/-/blob/master/debian/patches/329.patch?ref_type=heads This closes raspberrypi/picotool#104
josch commented 2024-06-06 01:29:22 -05:00 (Migrated from github.com)

FYI, the pico-sdk autopkgtest failed on (big endian) s390x before but with the (rebased) patch from this pull request it passes now: https://ci.debian.net/packages/p/pico-sdk/testing/s390x/47384722/

FYI, the pico-sdk autopkgtest failed on (big endian) s390x before but with the (rebased) patch from this pull request it passes now: https://ci.debian.net/packages/p/pico-sdk/testing/s390x/47384722/
This pull request has changes conflicting with the target branch.
  • src/common/boot_picoboot/include/boot/picoboot.h
  • src/common/boot_uf2/include/boot/uf2.h
  • src/common/pico_binary_info/include/pico/binary_info/structure.h
  • tools/elf2uf2/CMakeLists.txt
  • tools/elf2uf2/elf.h
  • tools/elf2uf2/main.cpp
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'zeldin/endianness:zeldin/endianness'
git switch 'zeldin/endianness'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'develop'
git merge --no-ff 'zeldin/endianness'
git switch 'zeldin/endianness'
git rebase 'develop'
git switch 'develop'
git merge --ff-only 'zeldin/endianness'
git switch 'zeldin/endianness'
git rebase 'develop'
git switch 'develop'
git merge --no-ff 'zeldin/endianness'
git switch 'develop'
git merge --squash 'zeldin/endianness'
git switch 'develop'
git merge --ff-only 'zeldin/endianness'
git switch 'develop'
git merge 'zeldin/endianness'
git push origin 'develop'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bazel

   
bluetooth

   
board settings

   
boot_stage2

   
bug
Something isn't working

  
build

   
c++

   
can't reproduce
Can not reproduce the bug

  
clang

   
clib-interface

   
cmsis

   
code cleanliness

   
code size

   
compiler version issues

   
cyw43_driver

   
debug

   
discussion

   
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
dynamic_clock

   
elf2uf2

   
enhancement
New feature or request

  
errata

   
filesystem

   
flash layout

   
floating point

   
FreeRTOS

   
hardware_adc

   
hardware_claim

   
hardware_clocks

   
hardware_dma

   
hardware_exception

   
hardware_flash

   
hardware_gpio

   
hardware_i2c

   
hardware_irq

   
hardware_pio

   
hardware_pwm

   
hardware_rcp

   
hardware_rtc

   
hardware_spi

   
hardware_sync

   
hardware_uart

   
hardware_vreg

   
hardware_watchdog

   
hardware_xosc

   
help wanted
Extra attention is needed

  
host

   
IAR

   
installation

   
invalid
This doesn't seem right

  
linker scripts

   
lto

   
lwip

   
mbedtls

   
missing API

   
needs reporter detail

   
networking

   
newlib

   
newlib-multicore

   
non-c

   
non-cmake

   
non-gcc

   
openocd

   
orphan PR

   
OS integration

   
pico_async_context

   
pico_base

   
pico_binary_info

   
pico_flash

   
picolibc

   
pico_lwip

   
pico_malloc

   
pico_multicore

   
pico_printf

   
pico_runtime

   
pico_stdio

   
pico_stdio_usb

   
pico_sync

   
pico_time

   
pico_util

   
pioasm

   
port
issues or discussions of porting code/libraries to rp2040

  
question
Further information is requested

  
review

   
review2

   
rtos interop
Consideration for FreeRTOS co-existence

  
runtime init

   
static library

   
svd

   
test

   
tinyusb

   
vscode

   
wifi integration

   
wontfix
This will not be worked on

No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
Milestone
Clear milestone
No items
No milestone
2.4.0
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-raspberrypi/pico-sdk!329
No description provided.