OpenSourceEBike / TSDZ2_wireless

TSDZ2_wireless
36 stars 11 forks source link

removed icon (lower right) from garmin display, added walk mode from wireless remote #83

Closed rananna closed 3 years ago

rananna commented 3 years ago

@casainho , I have removed the unwanted icon in the lower right from the garmin display. @4var1 , the walk mode signal from the wireless remote is a simple on/off flag, when MINUS is held down ui_vars.ui8_walk_assist = 1;, when released, ui_vars.ui8_walk_assist = 0. My expectation is that walk assist comes on when MINUS is pressed and held down, and stops when the button is released. All walk assist settings should be handled by the android app.

That is not how it is currently working, as I think you have integrated walk assist levels with button changing, and the walk assist is timing out after a few seconds. Could you please have a look at this?

4var1 commented 3 years ago

Ok - that's odd - don't think I've changed anything there. I'm out at the moment - will be back in a few hours and should have some time to take a look.

On Mon, 15 Feb 2021 at 16:23, rananna notifications@github.com wrote:

@casainho https://github.com/casainho , I have removed the unwanted icon in the lower right from the garmin display. @4var1 https://github.com/4var1 , the walk mode signal from the wireless remote is a simple on/off flag, when MINUS is held down ui_vars.ui8_walk_assist = 1;, when released, ui_vars.ui8_walk_assist = 0. My expectation is that walk assist comes on when MINUS is pressed and held down, and stops when the button is released. All walk assist settings should be handled by the android app.

That is not how it is currently working, as I think you have integrated walk assist levels with button changing, and the walk assist is timing out after a few seconds. Could you please have a look at this?

You can view, comment on, or merge this pull request online at:

https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/83 Commit Summary

  • remove icon in garmin display
  • walk assist from remote

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenSourceEBike/TSDZ2_wireless/pull/83, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASRDIVW2P5IYNHEHYBTDV5TS7FDB5ANCNFSM4XU6TUGQ .

4var1 commented 3 years ago

@rananna Ok i thought you meant an issue with LEDs. What is the issue in more detail? I don't quite understand what the problem is. Yes I've enabled walk-assist with the wired buttons - but I don't do anything other than set ui_vars.ui8_walk_assist in the same way as the 860c. It's all casainho's code. What do you mean all walk assist settings should be handled by the android app - as opposed to what?

Sorry if I'm being obtuse - it's not intentional :)

4var1 commented 3 years ago

bool anyscreen_onpress(buttons_events_t events) { if ((events & DOWN_LONG_CLICK) && ui_vars.ui8_walk_assist_feature_enabled) { ui_vars.ui8_walk_assist = 1; led_clear_queue(); led_hold_queue(); led_alert(LED_EVENT_WALK_ASSIST_ACTIVE); return true; }

4var1 commented 3 years ago

Then this is called periodically to handle the state:

void walk_assist_state(void) { if (ui_vars.ui8_walk_assist_feature_enabled) { // if down button is still pressed if (ui_vars.ui8_walk_assist && buttons_get_down_state()) { ui8_walk_assist_timeout = 2; // 0.2 seconds } else if (buttons_get_down_state() == 0 && --ui8_walk_assist_timeout == 0) { led_release_queue(); ui_vars.ui8_walk_assist = 0; } } else { ui_vars.ui8_walk_assist = 0; } }

4var1 commented 3 years ago

Ugh - what is the point of the code tags - they do an appalling job!

4var1 commented 3 years ago

Oh ok i think I see what you mean. You don't want the controller to handle the button time-out and disable walk assist?

Why is that only handled by the app? That's not good... Means we can't have the system running 'headless'...

The app should only be a UI surely? It doesn't play a part in any control I thought - its functions are to display things and enable changing config of the settings stored in the controller.

@casainho - what's the architecture?

4var1 commented 3 years ago

I can work around it by setting a flag and only process the timeout if the button was pressed on the wired remote. I'll give that a try later and report back.

rananna commented 3 years ago

Well, I never used the 860 for walk assist, so I really don't know how it works. My expectation is that you would use the android app to set the walk assist motor speed. Once that is done, a user would simply press and hold to start the assist, and the motor would walk the bike uphill for them. When they release the button walk assist would stop.

When I set the variable in the code, the motor turned on walk assist, but it timed out after ?15? (Didnt get exact timing) Seconds, even though I didn't release the button. If I release the button before this timeout walk assist stopped as expected.

rananna commented 3 years ago

The walk assist motor speed is indeed a 'config' setting. once the user sets it for their desired walk speed, they never touch it again.

4var1 commented 3 years ago

Oh - I assumed it timed out after a much shorter period... 0.2s as per the code above. I don't think that's the wireless controller doing that then... puzzling

rananna commented 3 years ago

Oh, I see the button intercept and timeout in the code, obviously this is not needed for the wireless 'walk' command

rananna commented 3 years ago

Why is there a timeout at all? Shouldn't walk mode stay active when the button is pressed and stop when the button is released?

rananna commented 3 years ago

A 'dead man' switch if you will.....:)

4var1 commented 3 years ago

i don;'t see any code in the wireless controller that sets ui_vars.ui8_walk_assist from a data packet from the app - that state is local to the remote/app. So i don't think the wireless controller is processing your walk assist state...

The timeout is after button released - it keeps running walk assist for 0.2s. Don't ask me why :) it's all code from the 860c - almost all of the 'wired remote' is copy/paste from the 860c codebase to keep existing functionality - and use proven code...

rananna commented 3 years ago

@casainho, we need your assistance on this......

4var1 commented 3 years ago

I was just testing walk assist on my dev setup without a motor - doesn't seem to timeout.. as an aside did you also change the sequence that plays on walk assist - the LED is on almost all the time. It might have been like that before but I reckon having a 200mS off period is better power saving that the current 50mS. But I also think I need to stop changing led sequences :)

4var1 commented 3 years ago

From how you've described it I think it's an issue in the remote. All the above code only fires if ui_vars walk assist changes, not rt_vars - and rt_vars is updated by ui_vars not the other way around. So the ui_vars setting is local to the controller...

rananna commented 3 years ago

Yeah, it is on almost constantly. I wanted to distinguish from other modes. Since walk more is unlikely to be used frequently, or for long time during a ride, I was less concerned about pwr consumption.

Let's freeze signalling until I can do a more in depth power study. If they turn out to have a big power impact, it is easy to fix. Generally, most of the battery consumption occurs when you are NOT riding the bike. A spst switch built into the box would REALLY improve battery life!

4var1 commented 3 years ago

In other words - how I read the code in the 'wired remote' is that it will only disable walk assist if walk assist was enabled on the 'wired remote'...

rananna commented 3 years ago

The timeout is probably due to the fact that I am not using the button code and simply setting the variable true or false.

4var1 commented 3 years ago

Yes sure I won't change any more LEDs for now - really interested to see how the power stuff stacks up. I do get a bit obsessive but I love UI design and information theory - and the two are what led sequences are all about essentially. Like changing the battery soc to red/yellow/green flashes - means the first flash contains 30% of the information instead of <=10%... anyway I digress!

rananna commented 3 years ago

Again, all I am doing is setting ui_vars.ui8_walk_assist = 1 when button is pressed, when released, ui_vars.ui8_walk_assist = 0.

4var1 commented 3 years ago

yes i get that - but I'm saying that the wireless controller button code only processes the ui_vars. version of the walk state. That is not updated from any packets from the app or otherwise.

So the walk assist state code on the wireless controller has no idea that the remote has pressed any buttons or is doing walk assist... from what I read.

4var1 commented 3 years ago

i can't test it as I don't have a wireless remote - but you can easily comment out the walk assist code from the wireless controller and see if the problem goes away or persists....

4var1 commented 3 years ago

After 15s: does the motor stop doing walk assist - or your led sequence stops flashing - or both?

rananna commented 3 years ago

The motor stops. As the led sequence is handled in the wireless remote code, it continues on until I release the button.

4var1 commented 3 years ago

how odd - as I say I really don't think the 'UI part' that writes to ui_vars. of the wireless controller is aware of that - and it doesn't process any timeout on the rt_vars data.

Comment out the code I added to the wireless controller and see what happens - it's just a few lines in the main loop.

4var1 commented 3 years ago

lines 1999 onwards - comment these 4 lines (alternatField() should be already commented)
//walk_assist_state(); //handle_buttons(); //alternatField(); // Removed until we can resolve what to do with the alternate state display requirements //streetMode();

4var1 commented 3 years ago

Any insights? I'm off to bed - if the weather is better again here tomorrow I'll flash the latest fw onto my bike and go for a little ride to test things without the remote attached. I should've gone today as it was the warmest in a long time but didn't have the time alas...

casainho commented 3 years ago

Walk assist has a kind of delay before stop, so if you are over rocks and there is vibrations and you momentarily release the button, the walkassist will keep running / button state will keep.

rananna commented 3 years ago

@casainho, I am finding the walk assist is stopping after a few seconds. I simply set ui_vars.ui8_walk_assist = 1 when MINUS is held down, when released, ui_vars.ui8_walk_assist = 0. Can you suggest a reason for this?

casainho commented 3 years ago

@casainho, I am finding the walk assist is stopping after a few seconds. I simply set ui_vars.ui8_walk_assist = 1 when MINUS is held down, when released, ui_vars.ui8_walk_assist = 0. Can you suggest a reason for this? I do not know. Maybe this issue is the same as I keep loosing the assistance a few times in my rides... when I loose it, if I increase or decrease the assist level, the motor assistance is back.

You can try to see the motor state errors, just send that variable as hall sensors variable and see on the mobile app. I would check motor state errors.

4var1 commented 3 years ago

I just put the latest fw on my wireless controller on my bike - only tested on a stand - but walk assist works ok.

The motor obviously goes on/off when the wheel exceeds the speed limit - but it kicks in again once the wheel speed drops. Kept going for a minute or so before I got bored! :)

@rananna - did you comment out the walk assist/button code in the wireless controller just to confirm it's not something that I've missed?

4var1 commented 3 years ago

@casainho, I am finding the walk assist is stopping after a few seconds. I simply set ui_vars.ui8_walk_assist = 1 when MINUS is held down, when released, ui_vars.ui8_walk_assist = 0. Can you suggest a reason for this? I do not know. Maybe this issue is the same as I keep loosing the assistance a few times in my rides... when I loose it, if I increase or decrease the assist level, the motor assistance is back.

You can try to see the motor state errors, just send that variable as hall sensors variable and see on the mobile app. I would check motor state errors.

This sounds like the issue I had where the IO pins were picking up noise/interference that registered as button presses.

So i reworked the button press code so that it required the press to be 50mS in length before it registered... That fixed my issues where assist would change wildly when the motor ran. It typically manifested itself in more down presses than up - so if you weren't looking at the app it seemed like assist was just disabling.

The assist changes would show on the android app however - so if you don't see that - then maybe it's not the same issue.

casainho commented 3 years ago

At least in Garmin I saw the assist level the same and when I increased or decreased, I saw the assist level changing as expected and not starting from 0.

rananna commented 3 years ago

Strange issue. I will do more testing with walk assist today .

rananna commented 3 years ago

At least in Garmin I saw the assist level the same and when I increased or decreased, I saw the assist level changing as expected and not starting from 0.

That would seen to eliminate remote button press issues.

4var1 commented 3 years ago

went out for an hour ride since the sun came out for a bit and it's nearly 10c here today :) All works fine from an assist point of view - totally forgot to test walk assist though :( does work fine on the stand though as mentioned...

4var1 commented 3 years ago

Not that it makes an odds - but I had nothing attached - no phone or other display.

Did mean I got lost though!

rananna commented 3 years ago

@4var1, the problem with the wireless remote is that you call walk_assist_state in the main loop, and if the button connected to the board is off, you turn off walk assist. see: void walk_assist_state(void) { // kevinh - note on the sw102 we show WALK in the box normally used for BRAKE display - the display code is handled there now if (ui_vars.ui8_walk_assist_feature_enabled) { // if down button is still pressed if (ui_vars.ui8_walk_assist && buttons_get_down_state()) { ui8_walk_assist_timeout = 2; // 0.2 seconds } else if (buttons_get_down_state() == 0 && --ui8_walk_assist_timeout == 0) { led_release_queue(); ui8_walk_assist ui_vars. = 0; } } else { ui_vars.ui8_walk_assist = 0; }

This means that when the remote turn on walk assist by setting ui8_walk_assist ui_vars=1, you immediately turn it off in the main loop as the button connected to the main board is in the up state. There could be many ways to fix this, but rather than me messing around with your button code, could you please add an ON/OFF walk assist flag for me to use that plays well with your button code?

rananna commented 3 years ago

@4var1, also the board remote plus key seems to be adjusting walk assist speed levels. Not sure how this speed adjustment worked with the screen. I think this needs to be moved to the android app.

4var1 commented 3 years ago

Did you comment out the walk_assist code like I suggested to make sure that is the cause?

Reading the code I cannot see how I would see your walk_assist state from the wireless controller. I looked into the code you highlight - but as I was trying to explain yesterday - my ui_vars.ui8_walk_assist is local to the controller - it does not change when you set it.

I cannot find any code in the wireless controller that sets ui_vars.ui8_walk_assist from a received packet.

4var1 commented 3 years ago

@rananna i can only find code that sends that walk assist state to the motor via the UART. None of the sent or received ble packets contain the walk_assist_state from what I can see.

Please confirm that you still see this when you comment out the 4 lines I mentioned yesterday from the main loop for the wireless controller.

lines 1999 onwards - comment these 4 lines (alternatField() should be already commented) //walk_assist_state(); //handle_buttons(); //alternatField(); // Removed until we can resolve what to do with the alternate state display requirements //streetMode();

4var1 commented 3 years ago

i've just had another look - there's nothing in the ant lev pages - or any of the ble packets referring to the walk_assist_state that I can find.

Only references to the feature - and the levels.

Are you sure it just doesn't work at all? :)

rananna commented 3 years ago

The command is sent by ANT LEV and is in this PR, see: ` if (p_profile->page_16.current_rear_gear == 14) { // walk mode is activated ui_vars.ui8_walk_assist = 1;

}
if (p_profile->page_16.current_rear_gear == 15)
{
  // enable brakes: be as fast as possible
  nrf_gpio_port_out_clear(NRF_P0, 1UL << BRAKE__PIN);
}
if (p_profile->page_16.current_rear_gear == 0)
{
  //this state should clear both brakes and walk mode
  // disable walk mode
  // disable brakes: be as fast as possible
  nrf_gpio_port_out_set(NRF_P0, 1UL << BRAKE__PIN);
  ui_vars.ui8_walk_assist = 0;
}`
4var1 commented 3 years ago

it's not like that in main.c for the wireless controller. Can you please confirm that you see this with the 4 lines commented out!

if (p_profile->page_16.current_rear_gear == 0)
{
  //this state should clear both brakes and walk mode
  // disable walk mode
  // disable brakes: be as fast as possible
  nrf_gpio_port_out_set(NRF_P0, 1UL << BRAKE__PIN);
}
4var1 commented 3 years ago
if (p_profile->page_16.current_rear_gear == 14)
{
  // walk mode is activated
}
rananna commented 3 years ago

Also, you have this code imported that seems to change walk speed: `static bool onPressAlternateField(buttons_events_t events) { bool handled = false;

// start increment throttle only with UP_LONG_CLICK if ((ui8_m_alternate_field_state == 7) && (events & UP_LONG_CLICK) && (ui8_m_vthrottle_can_increment_decrement == 0)) { ui8_m_vthrottle_can_increment_decrement = 1; events |= UP_CLICK; // let's increment, consider UP CLICK ui8_m_alternate_field_timeout_cnt = 50; // 50 * 20ms = 1 second }`

rananna commented 3 years ago

look at the "files changed" for this PR. It seems to be there......

4var1 commented 3 years ago

mmmm.... i'm new to github so it could be me - but it tells me I'm 0 behind 'latest'...