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

Alpha release: discussion thread #3

Closed
geeksville opened this issue Jul 3, 2019 · 64 comments

Comments

@geeksville
Copy link
Contributor

commented Jul 3, 2019

Ok, so I've been looking around and before futzing with anything it would be great to get some feedback on approach. I also have a few questions:

  • It seems like a key approach on how to do this would be to refactor lcd_print() based on the LCD3 version (so most of the config stuff above it could initially or forever stay the same - so bug fixes features etc... share the same code)?
  • In the LCD3 case the display is an old school character style device with some semicustom bits added by the mfg? i.e. ui8_lcd_frame_buffer in that device is approximately characters. I would probably do something different for the bitmap ugui based device in the SW102.
  • It seems like the SW102 code is still pretty early days. i.e. there is a ugui proof of concept but the actual abstraction for drawing fields at positions and switching screens isn't yet in?

To do the config menu stuff (i.e. implement lcd_print() and associated goo for this new bitmap device) I'd kinda like to have a layer underneath it so I can call a function to say 'draw screen XYZ' where screen XYZ has fields A, B, C (each field with specified x,y pos, field width/height, font to use etc). And I assume for each screen we'd eventually want appearance goo around the fields (rounded borders etc...) and a choice to show some fields as little graphs or whatever.

Is something like this already a WIP? It seems like it would also be needed for stuff besides the config menus (i.e. it would be needed for the standard data screens the user sees while riding). If so, I'd like to use it - who should I talk with about this? If not, what is the general intention here? Do I need to write it?

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

(btw - this is all guessing because I don't have a LCD3 or a SW102 yet - I've never even seen one of these things in person :-) )

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

So, the design of the main screen is defined -- see the forum message. Also I did upload the design files to repository.

For configuration menu, I plan to implement the same as for 850C LCD -- see that I am not very experienced, maybe the quality of implementation I did is not very good but it works ok on 850C.

I would implement the main screen just like I also did on 850C.

My next step, that I did agree with Nick, is to implement the configurations screen.

If you can, please implement main screen following the design file. I suggest to follow 850C code.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

sounds good I'll look for that forum post and the 850C code.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

@casainho alas my searching is failing - can you point me at the location of the 850C code?

To summarize: I'll make the main screen code (and try to make the primitives somewhat reusable) based on your design images (that I've now looked at). Alas though my SW102 might not be here for 3wks (which is what ali express is promising, but usually they are quite a bit earlier - probably more like 1 wk). So I won't be able to run the code I write to see if it works until then.

I will check it into a branch on my fork of this repo in case anyone wants to have a look.

(related: I just got the motor controller code to work nicely (enough to talk its serial protocol to the display - I bet the motor control loop is actually pretty unhappy ;-) ) on the sstm8 simulator. As soon as I get LCD3 code building, I'll also run that on the sim and connect their (simulated) serial ports together. I can send in a small PR for this if you want it)

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

Here the code for 850C: https://github.com/OpenSource-EBike-firmware/Color_LCD/tree/new_design/Bafang_color_LCD_850C/Bafang_LCD_850C_firmware/src

I don't know about using the simulator -- is there any advantage on this project??

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@casainho re simulator advantages
Currently for others probably not much. But for me (who hasn't got the hardware yet) it was useful. It allows me to spin up an instance of the LCD3 and motor controller software inside a STM8 simulators. Then hook them together so they are happily talking together.

Once my SW102 device arrives, I'll be able to hook it to my PC and debug/develop code for it without needing to take my motor off of my bike. ;-)

Someday it might also be useful if you want to have any sort of automated sanity test as part of the build.

My WIP branch which has this working is here. I'll link to the only relevant commit: geeksville/TSDZ2-Smart-EBike@3026454

@geeksville geeksville changed the title Adding config menu support: Concept review Adding main screen: discussion thread Jul 7, 2019
@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

@casainho Okay - I've looked through the 850C code and I think I have a pretty good handle on it. I'll implement the SW102 main screen per your design doc. I'll also implement a little/thin screen/fields layer just to keep a little bit of abstraction. Also you might find it will be useful for your config screen/menu stuff.

I'll use this issue to track progress but my current guess is:

  • My hardware arrives late this week.
  • I'll start coding when it gets here and will probably need about a week of evenings to have something others can use. I'll keep a WIP branch up on my github fork as I go.
  • I'll send in a pull request (and close this issue) when I'm finished (probably about a week after I get the hardware)

Cool?

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2019

will do! Alas, my stlink hasn't shown up yet (though my motor, battery, lcd3 and sw102 have) - my new guess is mon? It supposedly was given to the airline 7/3 but it is a discount chinese batch shipper so I bet it gets decontainerized somewhere here in the US and that hasn't happened yet.

On the plus side it gave me time to get the installation on my recumbent all sorted... I'll be able to do the SW102 coding on my computer+SW102+the sim motor controller, while I can use the LCD3/real motor on my test bike.
image

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

I used that kind of battery pack connector/support on the bicycle frame. Then after I just changed to use some sponge or any soft pad from local shop, to lodge the battery pack against the frame and used black strong large tape. I think works better because that space between the frame and battery connector/support is reduced at max as also the attachment will be very sturdy!!

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

update: alas, my stlink from china hasn't arrived yet- supposedly still in post somewhere inside the the US. I got impatient and just ordered anotherone from amazon, should get it tomorrow. ugh.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@casainho ok woot - I received my stlink from amazon (still waiting on the one from China). I happily loaded your firmware onto my motor and LCD3 and everything works swimmingly. I don't have to work tomorrow so I should be able to (finally) make good progress on the SW102 main screen. Hopefully by end of day I should at least have basic display of live data working.

Btw: the stlink clone I received from amazon initially did not work. After popping the PCB out of the case I discovered that the silkscreened pinouts on the case did not match the PCB ;-). If anyone else mentions DOA SDLINKs I bet that is the problem - not really DOA.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Good!! I hope you enjoy ;-)

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

@casainho ok my wip branch now seems to be happily displaying data for a mainscreen which approximates some of these fields:

https://github.com/OpenSource-EBike-firmware/SW102_LCD_Bluetooth/blob/master/design/Bafang_SW102_Bluetooth_LCD-design-proposal_1.jpg

One question - what is the 32 in this GUI? (I've never ridden with even my LCD3 yet so I have no idea)
edit: actually do you mind a complete approx list of what each of these fields are (I can map to the 850 code myself)?

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

update: never mind - I think I have it figured out from looking at the 850 code. Still lots to do, but things are kinda working. Next: hook it into real comms data, make the buttons work for assist changes, fix some draw artifacts to be less ugly, hook in the graph stuff...

Do you want me to also do the config stuff? I think I could bang it out pretty quickly after I finish this mainscreen.
image

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Great!!!!!!!

But please use the file for design v2, that is uploaded to the same folder as that one. V2 does not show a graph.

Yes, please finish if you can. I will not have time to work on this firmware.

On 850C, I used a copy structure of data to array using memcpy() and then write the array to eeprom. Maybe you can do the same, it will make easy to store the data from configurations menu. Even the variables from UART could be placed in array and then in structure this way (I didn't did yet on 850C).

@casainho casainho closed this Jul 25, 2019
@casainho casainho reopened this Jul 25, 2019
@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

wip here: https://github.com/geeksville/SW102_LCD_Bluetooth/tree/khwip

ok - I should have main screen mostly finished (including buttons etc...) in a couple of days. At that point I'll send in a PR then I'll do the config stuff, which I think will be another few days.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

You can take buttons code from LCD3 as the one from 850C is buggy write now.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@casainho what do you think of the state of the existing buttons code in the SW102 branch? did you write that and is it tested?

Similarly it seems like there has been quite a bit of divergence (or no shared history?) between the SW102 decode_rx_stream() and layer_2() in 850C. Was decode_rx_stream() in the sw102 ever used to talk to the 1.9 motor controller code?

I'm porting the state update/generation stuff from lcd_main_screen() now. And trying to decide how to untangle this. I've already split out the rendering from all of the 'business logic' of the lcd_main_screen subfunctions.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Sorry I don't know. IT was @lowPerformer that did that code, I don't remember last time I touched on this code.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

ok. I'll wait for @lowPerformer to chime in. If I run out of time since I know the 850C code talks to the 1.9 motor just fine (true?) I think in my refactoring I'll keep the meat of layer_2() instead of what is in decode_rx_stream().

I've already separated the various 850Cish layer_2() subfunctions and mainscreen stuff so that the GUI code is separate (and changed to use the new screen.c layer).

I'm trying to refactor this nicely so that there is hope that we can someday unify the 'business logic' of the 850C and SW102 codebases. Just changing the gui layout/button mappings between the two platforms. First I just want to get the SW102 approximately feature complete.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

btw - for a WIP snapshot, almost all of the huge swaths of what are now covered by #if 0/endif blocks in this file will go away (because screen/display/manipulation becomes nicely abstracted now):

for example see: https://github.com/geeksville/SW102_LCD_Bluetooth/blob/khwip/src/mainscreen.c#L724-L790

the new variant is not display dependent and the screen.h/screen.c layer takes care of always doing the minimum set of LCD screen draws.

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

Hello everyone,

decode_rx_stream() and the other stuff was tested for software version 0.18. Did the protocol change with latest firmware updates?

I use the button.c logic in other projects. But I don't know if this kind of logic is a feasible approach for this project. I just tinkered around with the nrf51 SDK...

BTW:
We should merge 12.3.0 branch with the master one, because the master is very old and uses a SDK version which I had trouble to work with the nrf51 chip.

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

Please be aware that I also changed LCD refresh logic in the uGUI library:

in ugui.c I added functionality for:
void UG_SetRefresh(void (*p)(void))
This sets a function which is called when ever the content was changed.

The pixel-set function, which is intitialized with UG_Init() just writes in an framebuffer and only when this is finished, the framebuffer wil be written to the LCD.

Please see lcd.c: lcd_init(void) for an example (pset() writes to framebuffer and lcd_refresh() transfers to LCD).

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

@lowPerformer thanks. I'm based on the 12.3.0 branch and my PR to master will also pull that in (in a few days? - TBD - def less than a week). My current TODO list is temporarily at /TODO.md in my wip branch. I should get the comms stuff mergemotized with the 850C based code tomorrow.

In screen.c I've turned off SetRefresh and instead do one flush at the end of each screen update (because a series of draw operations are often triggered by a new batch of state from the motor). This allows only one (expensive) push to the external oled.

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2019

If you need something done just give me a call. I am willing to help.

BTW:
Eclipse debug config from Windows should also work on Linux:
https://github.com/OpenSource-EBike-firmware/SW102_LCD_Bluetooth/wiki/Setup-IDE-(Windows)#openocd-debugging

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

The valuable feature for users is having the time but maybe it is not possible..??

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

So it is possible, you just have to count a variable ;)

But you don't have a backup battery in SW102 and you can't use the (low power optimized) RTC hardware because the low frequency oscillator is missing. So you have to:

  1. count with software timers (not that accurate but maybe is ok).
  2. have to set the clock every time the main battery is disconnected.

So if you ask me, save the clock implementation for later (we then sync with smartphone app) and just use timers like on LCD3.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

I think having time would be a distinctive feature. For as an user is very important as most part of time I run against the clock, daily at the city.

On 850C, there is a small drift and other than having minutes and hours variables that user need to setup, I think an offset is also need, maybe like 1/10 of second per day and so user can adjust over time.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

okay - I've added button presses (and I'm using the soft seconds tick for time for now ;-) ) and the assist and power buttons work correctly. (I'm temporarily using the M button as the power button - just so I can test/develop on my desk - the real power button sensing seems to depend on being hooked to a battery).

I need to work on some other stuff today, but tomorrow I'll add config menus as a nice little library/screen widget and have them set the various config settings.

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

... (I'm temporarily using the M button as the power button - just so I can test/develop on my desk - the real power button sensing seems to depend on being hooked to a battery)....

Thats is true. Without external power there is no high signal on the signal line.
I reverse engineered most of the circuits as you can see here btw:
https://github.com/OpenSource-EBike-firmware/Color_LCD/wiki/Bafang-LCD-SW102#integrated-ics

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

Oh, from a distance I see there is a high frequency (not the low frequency) oscillator installed! :)
So we maybe should change the clock settings in custom_board.h to use this!

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

Never mind! The curent configuration is correct and recommended!

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@lowPerformer oh yes thanks so much for doing all the hard work. I crib extensively from your schematic.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

ok I've added stub definitions for the little "config settings manager" which really turned into a "scrolling list selection widget, where elements in the list can be any field type defined in screen.h. Most commonly FieldEditable - a little widget that does nice editing of numbers/enums/bools."

And since the container for this list is just a regular screen component we can let the config menu open and still leave a few other widgets visible (battery level and time or whatever).

I think this will work really nicely and allow removal of lots of boilerplate code (and allow me to write/debug less code to get SW102 working). My initial test case is the wheel and battery config menus. Adding all the other menus will be super fast and easy. See this to get an idea of what it looks like:

https://github.com/geeksville/SW102_LCD_Bluetooth/blob/khwip/src/configscreen.c

I think the remaining code needed to actually make this new config system complete will be less than 200 lines of C code + about 80 lines of declarations for the various field to GUI mappings. Which is much shorter than the 850C version ( https://github.com/OpenSource-EBike-firmware/Color_LCD/blob/master/Bafang_color_LCD_850C/Bafang_LCD_850C_firmware/src/lcd_configurations.c ). And with no code dependencies on the layout/size of the LCD widgets.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

What I would like to change on configurations of 850C would be:

  • no small red triangle to point to active item on the menu but instead make that item blink and probably inverting the colors
  • make every section collapsible and user would first select a section that would then open and user then select the item of that section -- this way would be faster to navigate on the menu, because it is big and slow now
@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@casainho this would do both of those things super easily (and the second item you list is how I'm implementing it for SW102). I'm writing it all with the intent that after the initial SW102 version is released we can spend a small amount of time and merge much of the 850C and SW102 codebases. So the 'backend logic' of how a vehicle works and what config options are stay the same. The only thing that changes is a HALish layer for different ways of talking to hardware and remixing the same UX fields into different screen layouts.

Another cool thing about this screen abstraction is that we could someday have different screens for different modes (i.e. the standard main screen, a screen that graphs power and a few other things and a config screen).

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

Sweet

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

alas, yesterday and today I mostly needed to work on other $ stuff. So my estimated dates will get pushed out two days. Config menus now kinda work, but the actual elements don't yet have the editing code. I'll add that tomorrow.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

okay @casainho merged (woot!) my current WIP (I'll keep working through TODO items on my branch) to the 12.3.0 branch. Comments here:

#9

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

btw - I'm still making a fair number of changes (working through that TODO list and getting pretty close to an alpha release). But I'll avoid spamming with PR requests until it is feature complete. Anyone who wants to see current WIP it is here:

https://github.com/geeksville/SW102_LCD_Bluetooth

@geeksville geeksville changed the title Adding main screen: discussion thread Alpha release: discussion thread Aug 2, 2019
@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

@casainho so today I'll probably have time to change the mainscreen to almost exactly match https://github.com/geeksville/SW102_LCD_Bluetooth/blob/khwip/design/Bafang_SW102_Bluetooth_LCD-design-proposal_2.jpg

What are the 150, 325 and 525 fields supposed to represent?

Is it okay if I change the speed field to be white on black? In actually looking at it in my hand I think big blobs of white look not so great on the b/w OLED - particularly outdoors. I'd also like to change the config menus so the heading is white on black but with a nice white dividing line.

I'll also make a release makefile rule because I think an alpha build is almost ready (we can distribute a .hex for initial programming and a zip of the OTA bluetooth update file).

@casainho

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

Top field is motor/battery power and when clicking assist level, it shows current value multiplier and increases/decreases the value like a constant value of +-1.20 (20%) each click and shows the assist multiplier for something like 5 seconds than go back to show the motor/battery power.

Bottom field is customized by user and can be any variable like motor temperature, pedal human power, pedal cadence, etc. Maybe click long click of on/off button to let user change the variable with up/down buttons and long click on/off to exit this mode.

No problem to change the colors. I expected wheel speed to had more highlight like that.

Motor power is limited when hitting max power, max current, max ERPs(motor RPM of 4000) and max PWM duty_cycle of 254. Maybe the motor/battery power field could invert colors on this. Blinking is already used to limit max power configuration.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

thanks. I'll do that. Also I just rode my first ride with your stuff and it worked great! On my bike I installed an LCD3 that I had ordered but I can already tell I'll be ordering a 850C today (for futzing) and a SW102 (so I can install it, and keep my butchered one for development). The SW102 will fit nicely on my under seat steering, because the stock location to install a 850C/LCD3 is only really visible if I stop the bike and turn the handlebars.

I'll change over to the final mainscreen layout today and fix some ugliness in the menus.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I think the TODO list for the alpha release is getting short. Probably done in a few days (alas, I've been busy with work). Any feedback welcome.

https://github.com/geeksville/SW102_LCD_Bluetooth/blob/khwip/TODO.md

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

@geeksville
Some short questions:
I am looking in the uart.c to implement some kind of RX ringbuffer. I see that the CRC is checked in the ISR. Do you guys think the ISR is a good place to do this quite expensive task? Maybe better to put this in the decoding routine? I tend to keep ISRs as stupid as possible.

In main.c there is a FIXME comment in the init_app_timers. APPTIMERS work for me without this LFCLKSTART init and we even don't have a LFCLCK (no corresponding oscillator installed). You ran into problems without this?

Edit:
ARGH, I always get confused by this LFCLCK. We set Low frequency clock source in custom_board.h to internal RC. But for me it works without LFCLKSTART init.

@casainho

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

See that motor controller only sends data every 100ms, I don't think that makes much difference if the time used to process is in or out of the interrupts. But I think one way or the other it will work. One thing is that SW102 runs at 16Mhz and the 850C at 108Mhz, so maybe it is not irrelevant where the processing happens...?

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

lfclkstart: Yes, SoftDevice needs and starts LFCLCK. As soon as you comment out ble_init, APPTIMER indeed doesn't trigger any more without the extra init code!

CRC:
I think it is just "Good Programming Practice" to keep ISR as clean/short as possible. Although we don't have any issues here right now, it is better to place this in the app code imho.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

re: ISR
totally agree, and even if we have extra CPU cycles sitting around it still helps latency for other ISRs to keep ISR tiny tiny. easier to debug stuff outside of ISR anyways.

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

btw: my second sw102 arrived and I'll be switching from my LCD3 on my bike to actually using this pile of code on a vehicle tomorrow. So if I stop posting you can presume I died. ;-)

@lowPerformer

This comment has been minimized.

Copy link
Collaborator

commented Aug 7, 2019

:)
May your Gods be with you ;)

@geeksville

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

(moving future discussion to gitter)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.