New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Task 16 backend #52

Open

danielolssonIT wants to merge 6 commits into ItJustWorksTM:devel from danielolssonIT:task-16-rebased

Contributor

danielolssonIT commented Nov 25, 2021 •

edited

Loading

Implementation of Task 16 ArduinoGraphics library support in libSMCE by Group 1 (Daniel and Erik).

Task description: Add to libSMCE the ability to draw on an output screenbuffer, and add to the frontend a display screen component.

Our approach was to create a FrameBufferAccess class to be used in sketches in Arduinoland to access the backend framebuffer.

This branch is currently not working on Windows, as it is branched off from the master branch where the boost libraries seem to fail on Windows. If you want to try out a prototype that works on Windows, try the danielolssonIT:task-16-fix branch.

Erik Bock and others added 6 commits

November 25, 2021 14:57


          Re-added all the backend functionality for the frame buffers after th…

e7139ab

…e boost issues had been resolved


          Changed the framebuffer key for the accessor to not collide with the …

4572be8

…camera


          Change FramebufferAccess key back to 0

555cd1a


          Add missing limits include

77d1564


          Apply ClangFormat to WString.h

a3243de


          Change limits include to climits for CHAR_BIT definition

d2d007d

codecov bot commented Nov 25, 2021 •

edited

Loading

Codecov Report

Merging #52 (d2d007d) into devel (68c4eb1) will decrease coverage by 2.98%.
The diff coverage is 1.23%.

@@            Coverage Diff             @@
##            devel      #52      +/-   ##
==========================================
- Coverage   64.95%   61.97%   -2.99%     
==========================================
  Files          81       82       +1     
  Lines        2968     3048      +80     
==========================================
- Hits         1928     1889      -39     
- Misses       1040     1159     +119

Impacted Files	Coverage Δ
include/Ardrivo/WString.h	`100.00% <ø> (ø)`
src/Ardrivo/FrameBufferAccess.cpp	`0.00% <0.00%> (ø)`
CMakeLists.txt	`94.21% <100.00%> (ø)`
CMake/BuildProfiles/UbuntuFocal.cmake	`0.00% <0.00%> (-100.00%)`	⬇️
CMake/BuildProfiles/DebianBuster.cmake	`0.00% <0.00%> (-100.00%)`	⬇️
CMake/BuildProfiles/UbuntuGroovy.cmake	`0.00% <0.00%> (-100.00%)`	⬇️
CMake/Modules/SetupBoost.cmake	`75.49% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68c4eb1...d2d007d. Read the comment docs.

RuthgerD reviewed

View reviewed changes

Contributor

RuthgerD left a comment

Seems ok overall, just a couple of small things

include/Ardrivo/FrameBufferAccess.h

Comment on lines +1 to +3

+              //
+              // Created by danie on 2021-11-02.
+              //

Contributor

RuthgerD Nov 25, 2021

Missing copyright

include/Ardrivo/WString.h

Comment on lines +184 to +186

+                  String& operator+=(const String& rhs) {
+                      concat(rhs);
+                      return (*this);

Contributor

RuthgerD Nov 25, 2021

Can't this be templated much like the concat function?

Member

AeroStun Nov 25, 2021

I second that

Member

AeroStun Nov 25, 2021

And why do all these return statements have their expression parenthesized?

src/Ardrivo/FrameBufferAccess.cpp

Comment on lines +1 to +3

+              //
+              // Created by danie on 2021-11-02.
+              //

Contributor

RuthgerD Nov 25, 2021

missing copyright

src/Ardrivo/FrameBufferAccess.cpp

+              #include <climits>
+              #include <iostream>
+              #include <SMCE/BoardView.hpp>
+              #include "../../include/Ardrivo/FrameBufferAccess.h"

Contributor

RuthgerD Nov 25, 2021

Shoudn't use relative paths

src/Ardrivo/FrameBufferAccess.cpp

+                      m_format = format;
+                      break;
+                  default:
+                      return error("Unknown format");

Contributor

RuthgerD Nov 25, 2021

Maybe be a bit more descriptive so that we know where this comes from

src/Ardrivo/FrameBufferAccess.cpp

+              extern void maybe_init();
+              } // namespace smce
+              int FramebufferAccess::begin(std::uint16_t width, std::uint16_t height, SMCE_Pixel_Format format, std::uint8_t fps) {

Contributor

RuthgerD Nov 25, 2021

shoudn't this also take they fb key? (even if its just 0 for now)

src/Ardrivo/FrameBufferAccess.cpp

+                  m_begun = false;
+              }
+              bool FramebufferAccess::read(void* buffer) {

Contributor

RuthgerD Nov 25, 2021

uint8_t* instead of void*

src/Ardrivo/FrameBufferAccess.cpp

+                  return true;
+              }
+              bool FramebufferAccess::write(void* buffer) {

Contributor

RuthgerD Nov 25, 2021

const uint8_t*

src/Ardrivo/FrameBufferAccess.cpp

Comment on lines +109 to +114

+              int FramebufferAccess::bitsPerPixel() const {
+                  if (!m_begun) {
+                      std::cerr << "FramebufferAccess::bitsPerPixel: device inactive" << std::endl;
+                      return 0;
+                  }
+                  return bits_bytes_pixel_formats[m_format].first;

Contributor

RuthgerD Nov 25, 2021

Shoudn't this be something on BoardView::Framebuffer itself? seems awfully useful thing to have and weird for it to be present in Ardrivo only

AeroStun requested changes

View reviewed changes

Member

AeroStun left a comment •

edited

Loading

Obvious copy/paste of the worst code you could find in the whole project. If I was a code duplicate detection tool I'd scream to be heard from the edge of the galaxy

include/Ardrivo/FrameBufferAccess.h

Comment on lines +10 to +35

+              enum SMCE_Pixel_Format
+              {
+                  RGB888, // RRRRRRRRGGGGGGGGBBBBBBBB // SMCE extension
+                  RGB444, // GGGGBBBB----RRRR
+              };
+              class SMCE__DLL_RT_API FramebufferAccess {
+                private:
+                  std::size_t m_key = 0;
+                  SMCE_Pixel_Format m_format = RGB888;
+                  bool m_begun = false;
+                public:
+                  int begin(std::uint16_t width, std::uint16_t height, SMCE_Pixel_Format format, std::uint8_t fps);
+                  void end();
+                  bool read(void* buffer);
+                  bool write(void* buffer);
+                  [[nodiscard]] int width() const;
+                  [[nodiscard]] int height() const;
+                  [[nodiscard]] int bitsPerPixel() const;
+                  [[nodiscard]] int bytesPerPixel() const;
+                  void horizontalFlip(bool flipped);
+                  void verticalFlip(bool flipped);
+              };

Member

AeroStun Nov 25, 2021

This is a terrible API
I can see that you copied it from the OV767X header, which is also a terrible API which I cannot alter for compatibility with third-party software.

Member

AeroStun Nov 25, 2021

Keep in mind that this is an internal header, meant for library patches to use. It is NOT for Arduino user code, and thus NEEDS to provide an equivalent of the FrameBuffer interface, not the shitty OV767X one

include/Ardrivo/WString.h

Comment on lines +184 to +186

+                  String& operator+=(const String& rhs) {
+                      concat(rhs);
+                      return (*this);

Member

AeroStun Nov 25, 2021

I second that

include/Ardrivo/WString.h

Comment on lines +184 to +186

+                  String& operator+=(const String& rhs) {
+                      concat(rhs);
+                      return (*this);

Member

AeroStun Nov 25, 2021

And why do all these return statements have their expression parenthesized?

AeroStun force-pushed the devel branch from f5e0731 to 3c5d0de Compare

March 21, 2022 00:28

AeroStun force-pushed the devel branch 11 times, most recently from 4d73b7e to 94f79af Compare

April 3, 2022 21:31

AeroStun force-pushed the devel branch 2 times, most recently from c30ed30 to ff1eeee Compare

April 4, 2022 15:18

AeroStun force-pushed the devel branch 7 times, most recently from 36c75b6 to e3b023f Compare

April 29, 2022 15:07

AeroStun force-pushed the devel branch 5 times, most recently from 83b8248 to c294c06 Compare

April 29, 2022 15:41

AeroStun force-pushed the devel branch 4 times, most recently from be750e6 to f7f05f0 Compare

November 5, 2022 23:18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet