olikraus / u8g2

U8glib library for monochrome displays, version 2
Other
5.17k stars 1.05k forks source link

why rely on __ARM_LINUX__ for U8G2_USE_DYNAMIC_ALLOC #2257

Open giuliomoro opened 1 year ago

giuliomoro commented 1 year ago

Since dc14b93, u8g2.h has been changed so that it requires the __ARM_LINUX__ macro to be defined in order to enable U8G2_USE_DYNAMIC_ALLOC. There are a few weird things happening here:

I have to ask what is the rationale for not relying on the compiler's built-in __linux__ or __unix__ and the likes ? In case it is deemed undesirable to rely on these, so that the feature can be switched off even on Linux/Unix platforms, then why not define U8G2_USE_DYNAMIC_ALLOC directly in cmake instead of __ARM_LINUX__, or at least use a macro that doesn't start with __ ?

olikraus commented 1 year ago

As a project maintaner, i unfortunately have to say, that i almost ignore this feature. I just more or less blindly include PRs for this topic. Looking into the above mentioned commit, the correct thing would be to include this check additionally instead of replacing the check for the existing macros.

giuliomoro commented 1 year ago

As a project maintaner, i unfortunately have to say, that i almost ignore this feature.

do you mean the U8G2_USE_DYNAMIC_ALLOC or the __ARM_LINUX__ feature?

olikraus commented 1 year ago

The dynamic alloc topic.

I mean, i don't remember why and what was changed there. However, if you need this dynamic alloc topic, then you could just define U8G2_USE_DYNAMIC_ALLOC at compiler level with -DU8G2_USE_DYNAMIC_ALLOC or something.

giuliomoro commented 1 year ago

yup, that's what I ended up doing, but the awkwardness of the __ARM_LINUX__ macro is what prompted the creation of this issue.

As for investigating the reasoning behind the commit, would you happen to know who is the "servadmin servadmin@ubuntuserv" that authored it?

Overall, what made me struggle the most was figuring out that the drawing buffer is by default shared between objects, which is unexpected, considering that the interface is otherwise object-oriented. I very well see the reasons for such a design decision when the code runs on microcontrollers where every RAM byte is precious, but on Linux it goes at least against the principle of least surprise. This point is probably tangential to the one mentioned in the issue's title, though, apologies for the digression.

olikraus commented 1 year ago

As for investigating the reasoning behind the commit, would you happen to know who is the "servadmin servadmin@ubuntuserv" that authored it?

There should be a related pull request. Indeed if I remember correctly I didn't want an automatic activation of that dynamic memory at all.

buffer is by default shared between objects

Yes, i know. In fact initially u8g2 was not designed to support multiple displays in parallel. The other point: I wanted to have static allocation only. I mean, i wanted to avoid any malloc(). So I ended up in sharing the buffer between objects.