Skip to content
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

[WIP] Improve knightos/display.h #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aviallon
Copy link
Member

@aviallon aviallon commented Jun 6, 2020

Implemented several functions in C since I don't know if the kernel supports it natively.

Implemented in C since I don't know if the kernel supports it natively.
@aviallon aviallon changed the title [WIP] Improve knightos/display.h Improve knightos/display.h Jun 6, 2020
@aviallon aviallon marked this pull request as ready for review June 6, 2020 01:17
Copy link

@pixelherodev pixelherodev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, it'd probably be better to write this in asm and expose a small C interface. SDCC 4 is a lot better with codegen, but it still reeks compared to hand-written (and we're not on it, just yet).

If you'd rather not redo this, I get it, and I'm happy to merge as is after testing a bit.

src/knightos/display.c Outdated Show resolved Hide resolved
}

void draw_float(SCREEN* screen, unsigned char x, unsigned char y, float value){
/* Implementation is weird and slow because of non-working snprintf. Indeed, %f format strings are not yet implemented */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aviallon what was wrong with snprintf?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%f doesn't work at all (it's ignored). I don't know if it is standard, but every libc I know supports it. And it really is useful.

Copy link
Member

@MaxLeiter MaxLeiter Jun 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why we didn't implement %f, and there's probably a good reason, but you may want to experiment with https://github.com/KnightOS/libc/blob/master/src/format.c

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worst case, I'd like to see %f support using this function as a base.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this function has a major issue :
it does not support floats whose integer part is greater than INT_MAX...
In order to have full float support, I would have to go char by char... which may be even slower 😞

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this PR is done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I will do. If you have any idea on a fast implementation for that, I'm all ears 😝

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First idea: don't worry about fast. It can be sped up later if we need to.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this is ready for another review :)

@aviallon
Copy link
Member Author

aviallon commented Jun 6, 2020

I may try to do the signed integer one in ASM. But for floats it may be a little hard.

@MaxLeiter
Copy link
Member

@aviallon fyi @pixelherodev and I are active on IRC. #knightos on freenode.net

@aviallon
Copy link
Member Author

aviallon commented Jun 6, 2020

@aviallon fyi @pixelherodev and I are active on IRC. #knightos on freenode.net

Are you interested in a Matrix group? (IRC bridges are easy to set up and officially supported)

I'm coming soon.

@pixelherodev pixelherodev changed the title Improve knightos/display.h [WIP] Improve knightos/display.h Jun 12, 2020
@pixelherodev
Copy link

Needs to be updated now that e.g. log10 is in libc.

@aviallon
Copy link
Member Author

aviallon commented Jun 14, 2020

WTF happened (somehow I had random commits in my PR)

@aviallon aviallon force-pushed the patch-display-improvement branch from 6355594 to a35754c Compare June 14, 2020 20:03
@aviallon aviallon requested a review from pixelherodev June 15, 2020 08:19
@aviallon
Copy link
Member Author

Updated to use log10u. I am in the process to try to make a draw_float able to display any floats, but it certainly is not trivial ! I got some ideas though...

include/knightos/display.h Show resolved Hide resolved
* Draws a long at x, y
* NYI

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO is better, it's more commonly used

value = -value;
}
draw_short(screen, x, y, value);
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the explicit return?

}

void draw_float(SCREEN* screen, unsigned char x, unsigned char y, float value){
/* Implementation is weird and slow because of non-working snprintf. Indeed, %f format strings are not yet implemented */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know when this is ready for another review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants