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

EE UART logging #171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

EE UART logging #171

wants to merge 1 commit into from

Conversation

israpps
Copy link
Contributor

@israpps israpps commented Nov 14, 2023

No description provided.

@israpps israpps force-pushed the patch-6 branch 4 times, most recently from cd61916 to 13c29b8 Compare November 14, 2023 13:59
@israpps
Copy link
Contributor Author

israpps commented Nov 14, 2023

Working as intended it seems
image

@Wolf3s
Copy link
Contributor

Wolf3s commented Nov 14, 2023

Working as intended it seems image

Tested on real hardware?

@israpps
Copy link
Contributor Author

israpps commented Nov 14, 2023

Working as intended it seems image

Tested on real hardware?

No need for it... but I can test it on my 70k with UART by Sunday

@h4570 h4570 added the enhancement New feature or request label Feb 11, 2024
@h4570 h4570 changed the title implement EE UART logging EE UART logging Feb 11, 2024
Copy link
Owner

@h4570 h4570 left a comment

Choose a reason for hiding this comment

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

Looks good, but real hardware

@@ -15,6 +15,10 @@
#include "time/timer.hpp"
#include "./version.hpp"

#define LOGGING_STDOUT (0)
Copy link
Owner

Choose a reason for hiding this comment

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

Please replace this C-Style code to C++ enums (like here).

_EESIO can be misleading for Tyra beginners, what do you think about LOGGING_UART?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please replace this C-Style code to C++ enums (like here).

_EESIO can be misleading for Tyra beginners, what do you think about LOGGING_UART?

Maybe. If we explicitly say this is for EE.
Because DECKARD slims also have an additional UART for their emulated IOP

Rn I don't have my PC at hand. Will fix later

if (Demo::IS_REAL_PS2_VIA_USB) {
options.writeLogsToFile = true;
options.loggingMode = LOGGING_EESIO;
Copy link
Owner

Choose a reason for hiding this comment

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

I think that we should keep, file logging as default.
You can consider add comment like // UART logging can be used via loggingMode = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. Probably left it like that while testing it

@@ -15,8 +15,9 @@
int main() {
Tyra::EngineOptions options;

options.loggingMode = LOGGING_EESIO;
Copy link
Owner

Choose a reason for hiding this comment

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

Remember to fix this duplicate

Comment on lines +26 to +31
#ifndef EESIO_UART_USE_SIOCOOKIE
sio_init(38400, 0, 0, 0, 0);
sio_putsn("TYRA: EE_SIO Enabled\n");
#else
ee_sio_start(38400, 0, 0, 0, 0, 1); // alternative wrapper. initializes UART, but also re-routes STDOUT and STDERR FILE* streams to EE_SIO
printf("TYRA: EE_SIO Enabled & STDOUT/STDERR hooked\n")
Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain this a bit more?
Correct me if I'm wrong:

  • If somebody will use LOGGING_EESIO, all stdout logs (printf) will by default go to UART? (#else) But we use sio_putsn() anyway?
  • What will happen if somebody will declare EESIO_UART_USE_SIOCOOKIE? (which is not defined by default?)

BTW I see that you have EESIO_UART_USE_SIOCOOKIE and EESIO_USE_SIOCOOKIE (different name) is it a bug?

@@ -10,6 +10,7 @@

#include "debug/debug.hpp"

bool EESIO_Initialized = false;
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about making this variable as private static? It will be inside TyraDebug then

@@ -63,7 +72,7 @@ class TyraDebug {
ss1 << "| Assertion failed!\n";
ss1 << "|\n";

if (Tyra::Info::writeLogsToFile) {
if (Tyra::Info::loggingMode == LOGGING_FILE) {
writeInLogFile(&ss1);
} else {
Copy link
Owner

Choose a reason for hiding this comment

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

Why LOGGING_EESIO is not here?

@Wolf3s
Copy link
Contributor

Wolf3s commented Feb 12, 2024

@israpps ☝️

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

Successfully merging this pull request may close these issues.

3 participants