Initial implementation of Smart Response XE display #673

twasilczyk · 2021-08-30T23:23:27Z

Implementation is monochrome only for now (the display is 4-level grayscale) until the new display APIs get merged from TomSaw's branch.

src/modm/ui/display/monochrome_graphic_display_horizontal_impl.hpp

examples/srxe/display/main.cpp

src/modm/board/srxe/board.hpp

src/modm/driver/display/st7586s_impl.hpp

twasilczyk · 2021-09-02T06:40:57Z

Okay, I think this is ready to go. I'm pretty happy with this code and with how your library supports good code.

Honorable mentions:

all commands+data is sent to the display via full structs and not a bunch of uint8_t's
no magic numbers, entire protocol is pretty self-explanatory (see initialization code)
LittleEndian<> and BigEndian<> templates, I always wanted to code them. See SetVop on how I used it.

Challenges:

it's not easy to run unit tests on Ubuntu (I have 20.04.2 LTS) which doesn't ship with gcc 10
avr-gcc shipped with modm doesn't support std::is_constant_evaluated (that's C++20 symbol)
why SPI::transferBlocking requires uint8_t* and doesn't just accept void* pointers? See sendCommand
is register type explosion expected? I got NLineInversion, NLineInversion_t, LineInversionType and LineInversionType_t for a single register
I'm still not sure why MonochromeGraphicDisplayHorizontal didn't work. I didn't dig deep though.

TomSaw

Just a quick look for now.
For consistence to ssd1306_register.hpp, st7586s_protocol.hpp should be st7586s_register.hpp.

src/modm/driver/display/st7586s_protocol.hpp

twasilczyk · 2021-09-02T15:49:12Z

Just a quick look for now.
For consistence to ssd1306_register.hpp, st7586s_protocol.hpp should be st7586s_register.hpp.

I also had this thought, but these are not registers (but commands and their payloads) and definitely not defines. Thoughts?

TomSaw · 2021-09-02T17:22:43Z

Just a quick look for now.
For consistence to ssd1306_register.hpp, st7586s_protocol.hpp should be st7586s_register.hpp.

I also had this thought, but these are not registers (but commands and their payloads) and definitely not defines. Thoughts?

Hardware-interfaces are likely a chaotic mix of registers, commands, protocols... more precise distinctions are taken up with the naming of enum's -> check other driver-headers!

I think it's an old convention to collect such things in one box called register for the benefit of consistence.

twasilczyk · 2021-09-02T21:56:47Z

Just a quick look for now.
For consistence to ssd1306_register.hpp, st7586s_protocol.hpp should be st7586s_register.hpp.

I also had this thought, but these are not registers (but commands and their payloads) and definitely not defines. Thoughts?

Hardware-interfaces are likely a chaotic mix of registers, commands, protocols... more precise distinctions are taken up with the naming of enum's -> check other driver-headers!

I think it's an old convention to collect such things in one box called register for the benefit of consistence.

modm doesn't seem to stick to a single name for this stuff. Interface definitions are scattered around files named: _defines, _transport, _register, _definitions, _data, _constants. register is only used by ssd1306 driver actually.

@salkinium, what's your take? I don't think this library should stick to old conventions, but that's not my call at the end of the day :)

TomSaw · 2021-09-03T19:00:35Z

Have proposed my best solution of grayscale-support for st7586 in #670

src/modm/driver/display/st7586s.hpp

twasilczyk

Have proposed my best solution of grayscale-support for st7586 in #670

I’ll debate a little on the details later, but agree overall. I’ll wait with grayscale mode until you merge your PR.

src/modm/driver/display/st7586s.hpp

TomSaw

Apologizes for the chopped review. I'm fighting with my IDEs github extension, for now falling back to the browser. So here's part 3/3 :P

I leave the review of your endianness-edits @salkinium cause haven't enough background.

TomSaw · 2021-09-04T05:55:19Z

src/modm/driver/display/st7586s_impl.hpp

+	modm_assert(x < Width, "st4586s.sc.x", "x >= Width", x);
+	modm_assert(x + w <= Width, "st4586s.sc.xw", "x + w >= Width", x + w);
+	modm_assert(y < Height, "st4586s.sc.y", "y >= Height", y);
+	modm_assert(y + h <= Height, "st4586s.sc.yh", "y + h >= Height", y + h);
+	modm_assert((x % pixelsPerByte) == 0, "st4586s.sc.x%", "x is not a multiply of PBB", x);
+	modm_assert((w % pixelsPerByte) == 0, "st4586s.sc.w%", "w is not a multiply of PBB", w);


These asserts evaluate all-over-again runtime :/

-> Why do you need setClipping to be public? Witch case requires the app-writer to call this directly?

Yeah, this is going to be slow. However, you can use modm_assert_continue_fail_debug, which will still evaluate at runtime, however only in debug profile. I think that's a good compromise to find nasty bugs.

Holy cow. Didn't you have a longer name for a regular assertion that doesn't get executed in debug mode? ;)

I was rather hoping for something along the lines:

CHECK(condition) - evaluates in debug build only

CHECK_ALWAYS(condition) - evaluates both in debug and release

Yeah, not happy about it either. Here's some explanation of what I'm trying to describe: https://modm.io/reference/module/modm-architecture-assert/#assertion-types

TomSaw · 2021-09-04T06:04:44Z

src/modm/driver/display/st7586s_impl.hpp

+void
+St7586s<SPI, CS, RST, DC, Width, Height>::initialize()
+{
+	namespace payload = impl::st7586s::payload;


You can place using payload = impl::st586s::payload within class declaration to spare repetitions.

I'm afraid you can't define a namespace inside a class.

error: 'payload' in namespace 'modm::impl::st7586s' does not name a type

src/modm/driver/display/st7586s_impl.hpp

TomSaw · 2021-09-04T06:09:10Z

src/modm/driver/display/st7586s_impl.hpp

+	RST::reset();
+	RST::setOutput();


RST::setOutput(false); // same result

Hah. I was actually hoping it would work this way, but the API scared me off. As it turns out, it wasn't API, but a mere implementation with swapped argument naming:

gpio_D4.hpp:

void setOutput(bool status);

While the TLD interface (gpio.hpp) has it:

void setOutput(bool value);

@salkinium: is this a typo? I was digging what status means and I had an impression it's something about pull-ups or port config.

It's a typo, it should reflect the architecture interface API.

src/modm/driver/display/st7586s_impl.hpp

TomSaw · 2021-09-04T06:27:30Z

modm doesn't seem to stick to a single name for this stuff. Interface definitions are scattered around files named: _defines, _transport, _register, _definitions, _data, _constants. register is only used by ssd1306 driver actually.

@salkinium, what's your take? I don't think this library should stick to old conventions, but that's not my call at the end of the day :)

There should be a standard!? I prefer _defines cause it takes the classic culture from ANSI-C. Anything else is already too tight.

TomSaw

_

src/modm/math/utils/bit_operation.hpp

src/modm/driver/display/st7586s_protocol.hpp

salkinium · 2021-09-04T11:20:25Z

src/modm/driver/display/st7586s_impl.hpp

+	modm_assert(x < Width, "st4586s.sc.x", "x >= Width", x);
+	modm_assert(x + w <= Width, "st4586s.sc.xw", "x + w >= Width", x + w);
+	modm_assert(y < Height, "st4586s.sc.y", "y >= Height", y);
+	modm_assert(y + h <= Height, "st4586s.sc.yh", "y + h >= Height", y + h);
+	modm_assert((x % pixelsPerByte) == 0, "st4586s.sc.x%", "x is not a multiply of PBB", x);
+	modm_assert((w % pixelsPerByte) == 0, "st4586s.sc.w%", "w is not a multiply of PBB", w);


Yeah, this is going to be slow. However, you can use modm_assert_continue_fail_debug, which will still evaluate at runtime, however only in debug profile. I think that's a good compromise to find nasty bugs.

src/modm/driver/display/st7586s_protocol.hpp

salkinium · 2021-09-04T12:02:12Z

avr-gcc shipped with modm doesn't support std::is_constant_evaluated (that's C++20 symbol)

GCC supports it since v9, so I suspect maybe a problem with combining constexpr and modm_always_inline?

why SPI::transferBlocking requires uint8_t* and doesn't just accept void* pointers? See sendCommand

void* just requires more casting for the average use-case, since you typically store buffers as uint8_t*.

modm doesn't seem to stick to a single name for this stuff. Interface definitions are scattered around files named: _defines, _transport, _register, _definitions, _data, _constants. register is only used by ssd1306 driver actually.

There isn't really a convention from my side. We typically allow the wild west to sprout until it creates enough annoyance to warrant a alignment to a convention. So far the convention has been: There should be only one header file per driver (ideally identically named as the lbuild module) that you can include that pulls in everything you need, the rest is implementation detail.

TomSaw · 2021-09-04T17:47:48Z

It's mainly just historic convention to have just one header file which pulls in everything that is needed. We can remove this construct completely and instead just #pragma once and then #include "st7586s.hpp", which will also make IDE code completion work in this file, since it's pulling in the correct definitions.

(Ok, this file is dependency free, so you don't need the #include "st7586s.hpp" part, but in the st7586s.hpp_impl.hpp file that can be used).

😋 Smaller Boilerplate... Wunderbar! Copy that

twasilczyk · 2021-09-05T20:46:57Z

avr-gcc shipped with modm doesn't support std::is_constant_evaluated (that's C++20 symbol)

GCC supports it since v9, so I suspect maybe a problem with combining constexpr and modm_always_inline?

Looks like it's just not there:

error: 'is_constant_evaluated' is not a member of 'std'

I know the root cause is likely in modm's avr-gcc fork and it's fixable there. But I'm already pretty deep in this rabbit hole :).

why SPI::transferBlocking requires uint8_t* and doesn't just accept void* pointers? See sendCommand

void* just requires more casting for the average use-case, since you typically store buffers as uint8_t*.

Does it? I'm pretty sure there's actually no casting needed in user code if the function accepts void* or const void*. Is there?

salkinium

I'm happy with this, thanks!

twasilczyk · 2021-09-07T06:56:17Z

Sorry for push+force push (the first one was a WIP commit on a new driver, the second was to remove it). Current state should be the same as approved.

salkinium · 2021-09-07T10:19:45Z

Whoops I wanted to rebased, but pushed my develop branch to your fork. I merged it manually.
Let's use feature branches in future ;-P

salkinium · 2021-09-07T13:19:59Z

I know the root cause is likely in modm's avr-gcc fork and it's fixable there. But I'm already pretty deep in this rabbit hole :).

No, it's probably an issue with our custom libstdcpp not having set the _GLIBCXX_HAVE_BUILTIN_IS_CONSTANT_EVALUATED define? It's a bit weird, since the lib config should enable it automatically? cc @chris-durand

chris-durand · 2021-09-07T16:59:49Z

@twasilczyk Did you include the type_traits header for std::is_constant_evaluated? I can't reproduce the issue.

twasilczyk · 2021-09-07T17:28:36Z

Huh, it was missing type_traits. Apparently when built for ARM, this header gets pulled in by some other header. I'll make a follow-up PR, but not today.

twasilczyk commented Aug 30, 2021

View reviewed changes

twasilczyk force-pushed the develop branch 3 times, most recently from c3e1336 to d12edd8 Compare August 31, 2021 06:43

salkinium added the feature 🚧 label Aug 31, 2021

twasilczyk force-pushed the develop branch 3 times, most recently from afb1bc4 to ab43133 Compare September 2, 2021 04:36

twasilczyk marked this pull request as ready for review September 2, 2021 05:14

twasilczyk force-pushed the develop branch 2 times, most recently from b020d1c to d057e27 Compare September 2, 2021 05:54

TomSaw suggested changes Sep 2, 2021

View reviewed changes

src/modm/driver/display/st7586s_protocol.hpp Outdated Show resolved Hide resolved

src/modm/driver/display/st7586s_protocol.hpp Outdated Show resolved Hide resolved

src/modm/driver/display/st7586s_protocol.hpp Outdated Show resolved Hide resolved

twasilczyk force-pushed the develop branch from d057e27 to 9584418 Compare September 2, 2021 21:26

salkinium self-requested a review September 3, 2021 06:31

TomSaw mentioned this pull request Sep 3, 2021

Support grayscale displays #670

Closed

TomSaw reviewed Sep 3, 2021

View reviewed changes

src/modm/driver/display/st7586s.hpp Outdated Show resolved Hide resolved

TomSaw reviewed Sep 3, 2021

View reviewed changes

src/modm/driver/display/st7586s.hpp Outdated Show resolved Hide resolved

twasilczyk commented Sep 3, 2021

View reviewed changes

src/modm/driver/display/st7586s.hpp Outdated Show resolved Hide resolved

src/modm/driver/display/st7586s.hpp Outdated Show resolved Hide resolved

twasilczyk force-pushed the develop branch from 9584418 to 8aaee52 Compare September 4, 2021 05:28

TomSaw suggested changes Sep 4, 2021

View reviewed changes

TomSaw reviewed Sep 4, 2021

View reviewed changes

salkinium reviewed Sep 4, 2021

View reviewed changes

twasilczyk force-pushed the develop branch from 8aaee52 to 64ce36f Compare September 5, 2021 20:31

salkinium approved these changes Sep 6, 2021

View reviewed changes

twasilczyk force-pushed the develop branch from 0050e2c to 64ce36f Compare September 7, 2021 06:55

salkinium closed this Sep 7, 2021

salkinium force-pushed the develop branch from 64ce36f to 462a29c Compare September 7, 2021 10:14

salkinium added this to the 2021q3 milestone Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of Smart Response XE display #673

Initial implementation of Smart Response XE display #673

twasilczyk commented Aug 30, 2021

twasilczyk commented Sep 2, 2021

TomSaw left a comment

twasilczyk commented Sep 2, 2021

TomSaw commented Sep 2, 2021 •

edited

Loading

twasilczyk commented Sep 2, 2021

TomSaw commented Sep 3, 2021 •

edited

Loading

twasilczyk left a comment

TomSaw left a comment

TomSaw Sep 4, 2021 •

edited

Loading

salkinium Sep 4, 2021

twasilczyk Sep 5, 2021

salkinium Sep 6, 2021

TomSaw Sep 4, 2021 •

edited

Loading

twasilczyk Sep 5, 2021

TomSaw Sep 4, 2021

twasilczyk Sep 5, 2021

salkinium Sep 6, 2021

TomSaw commented Sep 4, 2021 •

edited

Loading

TomSaw left a comment •

edited

Loading

salkinium Sep 4, 2021

salkinium commented Sep 4, 2021

TomSaw commented Sep 4, 2021 •

edited

Loading

twasilczyk commented Sep 5, 2021

salkinium left a comment

twasilczyk commented Sep 7, 2021

salkinium commented Sep 7, 2021

salkinium commented Sep 7, 2021

chris-durand commented Sep 7, 2021

twasilczyk commented Sep 7, 2021

Initial implementation of Smart Response XE display #673

Initial implementation of Smart Response XE display #673

Conversation

twasilczyk commented Aug 30, 2021

twasilczyk commented Sep 2, 2021

TomSaw left a comment

Choose a reason for hiding this comment

twasilczyk commented Sep 2, 2021

TomSaw commented Sep 2, 2021 • edited Loading

twasilczyk commented Sep 2, 2021

TomSaw commented Sep 3, 2021 • edited Loading

twasilczyk left a comment

Choose a reason for hiding this comment

TomSaw left a comment

Choose a reason for hiding this comment

TomSaw Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSaw Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSaw commented Sep 4, 2021 • edited Loading

TomSaw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salkinium commented Sep 4, 2021

TomSaw commented Sep 4, 2021 • edited Loading

twasilczyk commented Sep 5, 2021

salkinium left a comment

Choose a reason for hiding this comment

twasilczyk commented Sep 7, 2021

salkinium commented Sep 7, 2021

salkinium commented Sep 7, 2021

chris-durand commented Sep 7, 2021

twasilczyk commented Sep 7, 2021

TomSaw commented Sep 2, 2021 •

edited

Loading

TomSaw commented Sep 3, 2021 •

edited

Loading

TomSaw Sep 4, 2021 •

edited

Loading

TomSaw Sep 4, 2021 •

edited

Loading

TomSaw commented Sep 4, 2021 •

edited

Loading

TomSaw left a comment •

edited

Loading

TomSaw commented Sep 4, 2021 •

edited

Loading