Cxbx-reloaded: Add LEDs to our GUI

[PatrickvL]

The original Xbox has a multi-colored led surrounding the power button, which is capable to show 4 intensity levels of green, and 4 levels of red - for a total of 8 bits, and thus 256 combinations.

For this issue, come up with a graphical representation to show the color of the led somewhere in our GUI.

(Internally, it's being controlled via byte-writes to SMBus address 0x20, with the low nibble indicating the 4 green levels, and the high nibble indicating the 4 red states).

[ghost]

Alright, I am at a good point with this. I successfully extended the GUI of cxbx to display the LED on the menu bar and now I have to intercept the SMBus value but I have one question. I see the function of interest is HalWriteSMBusValue() (and its twin HalReadSMBusValue()) in EmuKrnlHal.cpp but in their current form they seem to only be able to handle the addresses 0xA8 and 0xA9 so I will also have to extend these functions to support the address 0x20 right?

[PatrickvL]

This has to be investigated first. Any information found, should be documented somewhere (xboxdevwiki perhaps), so whoever reads the code can verify it to the documentation)

[PatrickvL]

My compliments for picking this up btw!

[ghost]

You mean something like writing a custom xbe that calls those functions above and pokes the values in a real Xbox? If it takes only this then I think I can do this. BTW below there is a preview of my extended cxbx GUI. Is this design good enough?

[LukeUsher]

Keep in mind that although official Microsoft software will use HalWriteSMBusValue, homebrew could (and does!) write to the registers directly, so you would also have to handle this in EmuX86_Out();

[PatrickvL]

Actually, it would be best if smbus was emulated separately, like EPROM

[PatrickvL]

Also, it should be done via io read write handlers, which should be called from the kernel functions

[ghost]

Ok, I've done the testing and checked all the 256 possible LED color combinations. See the attached file for the details. Passing a value > 256 to HalWriteSMBusValue will cause the Xbox to repeat the color sequence from the beginning. It is also possible to send a negative number as a value: in this case the Xbox will just repeat the color sequence but in reverse order. This means that the last argument of HalWriteSMBusValue is probably a LONG instead of a ULONG.
Passing a boolean TRUE as third argument will cause an error independently of passing positive and negative numbers or null as a fourth argument. These are my test cases:

errCodeRegister = HalWriteSMBusValue(0x20, LED_SEQUENCE_REGISTER_CMD, TRUE, var);
Sleep(10);
errCodeMode = HalWriteSMBusValue(0x20, LED_MODE_CMD, FALSE, 0x01);  

errCodeRegister = HalWriteSMBusValue(0x20, LED_SEQUENCE_REGISTER_CMD, TRUE, var);
Sleep(10);
errCodeMode = HalWriteSMBusValue(0x20, LED_MODE_CMD, TRUE, 0x01);

errCodeRegister = HalWriteSMBusValue(0x20, LED_SEQUENCE_REGISTER_CMD, FALSE, var);
Sleep(10);
errCodeMode = HalWriteSMBusValue(0x20, LED_MODE_CMD, TRUE, 0x01);

In the first two cases errCodeRegister is 0 and errCodeMode is -1073741435 the 1st time running the test. If run a 2nd time, both codes are -1073741435. In the third case both error codes are 0 during the first run but both become -1073741435 afterwards. The Xbox LED still assumes the color expected from the var passed but upon rebooting it doesn't revert to the normal green color. For that, I have to shutdown and reboot the Xbox.
The only thing missing for the LEDs are the timings of the flashing but I have no way of measuring them. Unless we are going to use faked timings for now and correct them when we know their true values I am stuck with this now...
xbox led test.zip

[JayFoxRox]

You could use a camera to get a 30ms resolution, measuring a couple of runs you could improve the timing by averaging.
Anyhow, the LED has been documented for over 15 years: http://xboxdevwiki.net/PIC#The_LED
This behaviour is also emulated in Hackbox and XQEMU-JFR.

I've fixed your spreadsheet: xbox led test-fixed.xlsx

Also, if you have something you consider xboxdevwiki-worthy, just add it yourself. Accounts are free and we depend on decentralizing research (= many people contributing on their own, independently).

[PatrickvL]

@ergo720 Will you be coding this up, or are you leaving that to someone else?

[ghost]

@LukeUsher (or everyone else) do you know a homebrew with the source available that sets the registers directly instead of using HalWriteSMBusValue? I need to know this to proceed with the implementation.

[LukeUsher]

Just implement it inside HalWriteSMBusValue for now, we don't have enough low level stuff for direct IO writes to work properly yet (We don't emulate the SMBUS at all yet, or even IN/OUT instructions properly)

I'm working on LLE related things in my current branch, and it's likely I'll start real SMBUS emulation soon too, until then, HLE it in HalWriteSMBusValue.

[ghost]

I have finally implemented this feature, but before submitting the PR I'd like to ask about some issues I'm having. The 1st is that I am unable to test the implementation of HalWriteSMBusValue. If I am not mistaken (correct me if I'm wrong), this function is a kernel function, not an XTL one and so is not supposed to be called directly from the xbe and this should be why the led color always stays green (nothing is calling it to change the led). By the way, this is not a problem of the led code because I have tested the function I wrote for it in WndMain.cpp and the led flashes correctly. The 2nd one is that I chose a white color for the "off" led. I'd like to choose the background color of the menu bar, but depending on the theme used, the below two codes don't always retrive the correct color.

m_BrushWhite = CreateSolidBrush(GetSysColor(COLOR_MENUBAR));
------------
MENUINFO mi;
HMENU hMenu = GetMenu(hwnd);
mi.cbSize = sizeof(mi);
mi.fMask = MIM_BACKGROUND;
mi.hbrBack = m_BrushWhite;
GetMenuInfo(hMenu, &mi);

Finally, in win7, if using a non-aero theme, the bitmap used for the led appears in a slightly different position than expected and when clicked it changes color. This doesn't happen in win10.

[PatrickvL]

Kernel functions can be called from Xbe's - we 'just' have to find one that calls this API.

[ghost]

I tried my custom xbe I wrote to test the Xbox that calls this API but unfortunately cxbx crashes as soon as booting...

[PatrickvL]

As for the other questions, can't you find anything on stack overflow or a Google search?

[PatrickvL]

Having a working test case is important, indeed. Maybe someone can help who isn't as busy a Luke and me?

[LukeUsher]

A quick suggestion: Wouldn't be acceptable just to show the LED black if it is programmed to not be lit? I've seen other emulators with this behavior in the past, and it reflects what it would look like on real hardware too.

As for a test case: Yes it's a kernel function, but no commercial titles change the LED colour. It is mostly used internally by the Xbox operating system. That said, a lot of homebrew does so, the problem is finding one that does via this function rather than banging the hardware registers directly, and that also works in Cxbx-Reloaded, a custom test case xbe might be the best option.

As for your test xbe: Can you share the source code and a kernel log of execution? Perhaps we can figure out why it fails to run, if it works on real hardware but not Cxbx-Reloaded, that would be a bug.

[ghost]

Good news, I have rewritten my xbe and now it doesn't crash instantly as before. It still does after around 2-3 sec but that was already enough to see the flashing green when starting the test. To be honest, the 1st value tested is actually -1 and the LED is solid orange on the Xbox so this means the value has been interpreted as 1 instead. The return codes of HalWriteSMBusValue look correct as well. They are 0 with the implementation (2nd picture) and STATUS_UNSUCCESFUL without (1st picture).
KrnlDebug.txt
KrnlDebugLED.txt
Xbe.txt

[LukeUsher]

Your own source code is fine: So long as you don't share any Microsoft libraries or headers, that's how other projects handle this (XBMC, RetroArch for Xbox, etc)

[PatrickvL]

Pro-tip : Use Cxbx-Reloaded debug-builds to generate a lot more debug-output. (NOT XBE Debug builds, HLE can't handle those!)

In this case, that would show all calls and arguments to the Hal API's (amongst lots of other stuff, of couse).

[ghost]

Ok, I did as @PatrickvL said: I generated a debug version of the xbe but unfortunately it crashes instantly with a RtlAssert error. I am not sure about the debug log mentioned in the window though, cxbx has offered to me to save a "default.xbe" upon exiting and I did but I don't know what it is. Is it a copy of my xbe? Also, forgive the code, it's quite ugly because I just wanted to get something working quickly.
KrnlDebug-DEBUG.txt
Xbe-DEBUG.txt
Main.zip

EDIT: this log is with a debug version of Cxbx
KrnlDebug-CXBX_DEBUG.txt

[PatrickvL]

Sorry for the confusion (and I must admit I made a spelling error before), but I meant using a debug-build of Cxbx-Reloaded.

[PatrickvL]

Put the while-loop around the for-loop (instead of inside it), so that the XBE keeps running

[PatrickvL]

Better chat on https://gitter.im

[ghost]

Update: Avalaunch dashboard causes the LED to flash red and green, probably because is detecting some error condition in the emulated xbox that would cause the LED to flash. This means that we can also use this xbe to test the LED on the top of custom xbe's.

[PatrickvL]

Update: we now have the SMC implemented as a simple hardware device, that calls a function when the led color sequence is set. With that in place, the GUI can be updated based on that. See https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/blob/master/src/CxbxKrnl/SMCDevice.cpp#L144

[PatrickvL]

With the above, this work (setting the led sequence via EmuShared) can be done from the new SetLEDSequence function : https://github.com/ergo720/Cxbx-Reloaded/commit/aa5a9c643eb0ef3045c80c736bdb39bbbefa9c22#diff-9685dad3d112a0969981354eeb3be287R585

[PatrickvL]

... Which currently resides here : https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/blob/master/src/CxbxKrnl/CxbxKrnl.cpp#L788

[PatrickvL]

This issue can be closed, thanks to this pull request by @ergo720 : https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/874 (and some refactoring afterwards by yours truly)