-
Notifications
You must be signed in to change notification settings - Fork 2
Adding FB OLED Support #6
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.
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
base: master
Are you sure you want to change the base?
Conversation
Marking as draft until I test it all once more tomorrow |
Tested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating the separate commit for addressing comments, please rebase and squash changes and force push to the same branch and then create/update pull request
src/lcdlib/lcdlib_common_oled.c
Outdated
print_err("1 Writing string to display at (%d, %d): %s\n", x, y, buffer); | ||
|
||
// Maximum allowable string length | ||
int max_length = (OLED_WIDTH_PX - 7) / CHARACTER_WIDTH_PX; // 128 pixels, 6x8 glyphs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic number 7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating the separate commit for addressing comments, please rebase and squash changes and force push to the same branch and then create/update pull request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should take this opportunity to move away from CMakeLists and use Meson as upstream modules are using meson to speed up compilation.
BMC_VER, | ||
BIOS_VER | ||
} LCD_msgType_t; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int lcdlib_enable_dev(int lcdDeviceNum);
int lcdlib_disable_dev(int lcdDeviceNum);
would be required to extend beyond a single display/oled.
return 0; | ||
} | ||
|
||
int lcdlib_open_dev(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API signature needs change to extend beyond single buffer.
something like int lcdlib_open_dev(char *deviceName) or int lcdlib_open_dev(int deviceId)
|
||
return 0; | ||
} | ||
int lcdlib_close_dev(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API signature needs change to extend beyond single buffer.
something like int lcdlib_close_dev(char *deviceName) or int lcdlib_close_dev(int deviceId)
src/lcdlib/lcdlib_common_oled.c
Outdated
#define print_err(fmt, args...) | ||
#endif | ||
|
||
static int fb_fd = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Global file pointer will cause extensibility issues.
#endif | ||
|
||
static int fb_fd = -1; | ||
static unsigned char *fb_buffer = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to find a way to avoid global variables in general. This will come in the way of supporting more than one display.
|
||
int lcdlib_open_dev(void); | ||
int lcdlib_close_dev(void); | ||
int lcdlib_write_string(LCD_msgType_t msgType, unsigned char *buffer, int str_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a re-look to see if it needs to include deviceNumber as a parameter to write to the right display.
int lcdlib_open_dev(void); | ||
int lcdlib_close_dev(void); | ||
int lcdlib_write_string(LCD_msgType_t msgType, unsigned char *buffer, int str_len); | ||
int lcdlib_clearScreen(void); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a re-look to see if it needs to include deviceNumber as a parameter to clear the right display.
f1cbc1e
to
3f44707
Compare
This commit just adds the files to support the FB OLED device, 128x64px with each pixel being 1 nibble (4 bits), so FB is 64x64. Updated CMakeLists.txt to add option for OLED version Use -DUSE_OLED_VERSION=ON to build new OLED version (off by default)
3f44707
to
b1253f6
Compare
Added files to support FB OLED for future platform, and modified the CMakeLists.txt to add OLED build option.
To enable, use the cmake -DUSE_OLED_VERSION=ON flag (off by default). This will build the new version.
In bb recipie for amd-lcd-lib, will likely need to add EXTRA_OECMAKE += "-DUSE_OLED_VERSION=ON", and also remove DEPENDS += "i2c-tools" (this doesn't use i2c).