Skip to content

Commit

Permalink
test video checkpoint
Browse files Browse the repository at this point in the history
  • Loading branch information
@FMeshreky
FMeshreky committed May 30, 2024
1 parent e314d73 commit 552d170
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion embedded/OH2_Lower_Instrument_Panel/2A13_BACKLIGHT_CONTROLLER/2A13_BACKLIGHT_CONTROLLER.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,11 @@ void checkButtonPress() {
bool buttonPressed = digitalRead(SIMPWR_PUSH) == LOW;
if (buttonPressed && !buttonWasPressed) {
applyMode(0);
setBacklightBrightness(50);
if (brtns > 0) {
setBacklightBrightness(brtns);
} else if (brtns = 0) {
setBacklightBrightness(50);
}
encoder.setPosition(0);
}
buttonWasPressed = buttonPressed;
Expand Down

8 comments on commit 552d170

@kbastronomics
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is in error
bool buttonPressed = digitalRead(SIMPWR_PUSH) == LOW;

you're adding the value of SIMPWR_PUSH to buttonPressed then do a comparison to LOW ( == is a comparison check, = sets a value)
the == LOW should be removed as it's ambiguous at best

@kbastronomics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated, we need to either use adafruit_neopixel library or fastled can not have code for both.
My preference is for the adafuit library as it's easier to work with and understand for most users.
you don't have to understand arrays or indexes and manipulate them as you do in fastled.

@viajandee
Copy link

@viajandee viajandee commented on 552d170 May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This line is in error bool buttonPressed = digitalRead(SIMPWR_PUSH) == LOW;

you're adding the value of SIMPWR_PUSH to buttonPressed then do a comparison to LOW ( == is a comparison check, = sets a value) the == LOW should be removed as it's ambiguous at best

So this is a boolean variable "called" buttonPressed with two possible values, true, and false. On the right-hand side, I'm comparing the SIMPWR_PUSH (D24) current state to LOW which indicates 0v (grounded circuit) since it is a digital pin (normally the pull-up resistor keeps it in a HIGH state). When the button is pushed the comparison becomes true, "true" gets stored in the buttonPressed bool and then used in the rest of the code to do something. When released the comparison becomes "false" and the condition is no longer applied.

@viajandee
Copy link

Choose a reason for hiding this comment

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

Unrelated, we need to either use adafruit_neopixel library or fastled can not have code for both. My preference is for the adafuit library as it's easier to work with and understand for most users. you don't have to understand arrays or indexes and manipulate them as you do in fastled.

I'm using FastLED solely for the modes and effects otherwise everything is handled via the Adafruit library. I tried building the modes using Adafruit but it got very complicated very quickly to achieve some of the FastLED basic effects. This is the Backlight Controller after all and the backlight is its main function so we can use both if we have to, it won't impact any other function in the SIM. But I'm still working on refining the code after testing it so I will definitely look into that.

@kbastronomics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated, we need to either use adafruit_neopixel library or fastled can not have code for both. My preference is for the adafuit library as it's easier to work with and understand for most users. you don't have to understand arrays or indexes and manipulate them as you do in fastled.

I'm using FastLED solely for the modes and effects otherwise everything is handled via the Adafruit library. I tried building the modes using Adafruit but it got very complicated very quickly to achieve some of the FastLED basic effects. This is the Backlight Controller after all and the backlight is its main function so we can use both if we have to, it won't impact any other function in the SIM. But I'm still working on refining the code after testing it so I will definitely look into that.

You don't want to use both though, you only want one or the other on the buss at the same time as one will not know what the other has done on that buss and can throw things out of sync really easy.
one other the other, not both please.

@kbastronomics
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is in error bool buttonPressed = digitalRead(SIMPWR_PUSH) == LOW;
you're adding the value of SIMPWR_PUSH to buttonPressed then do a comparison to LOW ( == is a comparison check, = sets a value) the == LOW should be removed as it's ambiguous at best

So this is a boolean variable "called" buttonPressed with two possible values, true, and false. On the right-hand side, I'm comparing the SIMPWR_PUSH (D24) current state to LOW which indicates 0v (grounded circuit) since it is a digital pin (normally the pull-up resistor keeps it in a HIGH state). When the button is pushed the comparison becomes true, "true" gets stored in the buttonPressed bool and then used in the rest of the code to do something. When released the comparison becomes "false" and the condition is no longer applied.

But why even do this when digitalRead returns a LOW or HIGH already as seen here

int digitalRead(uint8_t pin)
{
	uint8_t timer = digitalPinToTimer(pin);
	uint8_t bit = digitalPinToBitMask(pin);
	uint8_t port = digitalPinToPort(pin);

	if (port == NOT_A_PIN) return LOW;

	// If the pin that support PWM output, we need to turn it off
	// before getting a digital reading.
	if (timer != NOT_ON_TIMER) turnOffPWM(timer);

	if (*portInputRegister(port) & bit) return HIGH;
	return LOW;
}

Also the defines for LOW, HIGH, TRUE FALSE are the same.
i.e.
LOW=FALSE=0
HGIH=TRUE=1

@viajandee
Copy link

Choose a reason for hiding this comment

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

This line is in error bool buttonPressed = digitalRead(SIMPWR_PUSH) == LOW;
you're adding the value of SIMPWR_PUSH to buttonPressed then do a comparison to LOW ( == is a comparison check, = sets a value) the == LOW should be removed as it's ambiguous at best

So this is a boolean variable "called" buttonPressed with two possible values, true, and false. On the right-hand side, I'm comparing the SIMPWR_PUSH (D24) current state to LOW which indicates 0v (grounded circuit) since it is a digital pin (normally the pull-up resistor keeps it in a HIGH state). When the button is pushed the comparison becomes true, "true" gets stored in the buttonPressed bool and then used in the rest of the code to do something. When released the comparison becomes "false" and the condition is no longer applied.

But why even do this when digitalRead returns a LOW or HIGH already as seen here

int digitalRead(uint8_t pin)
{
	uint8_t timer = digitalPinToTimer(pin);
	uint8_t bit = digitalPinToBitMask(pin);
	uint8_t port = digitalPinToPort(pin);

	if (port == NOT_A_PIN) return LOW;

	// If the pin that support PWM output, we need to turn it off
	// before getting a digital reading.
	if (timer != NOT_ON_TIMER) turnOffPWM(timer);

	if (*portInputRegister(port) & bit) return HIGH;
	return LOW;
}

Also the defines for LOW, HIGH, TRUE FALSE are the same. i.e. LOW=FALSE=0 HGIH=TRUE=1

This is the higher-level logic I opted for focusing on application specific logic rather than using the Arduino library to interact directly with the hardware and adapt the rest of the code to it, and in coding you can achieve the same thing in many ways, so both of the above lead to the same result. In this instance, I decided that booleans would be more efficient for the rest of my code structure.

@viajandee
Copy link

Choose a reason for hiding this comment

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

As I mentioned in the post in my build log, I have a couple of issues that I'm working on after testing, and checkButtonPress is one of them. So I might change it entirely but from my perspective I still prefer the higher-level logic for this purpose to simplify the rest of the code. We can have this discussion again soon when I submit the PR.

Please sign in to comment.