-
So i'll send a larger post after i'm done work .
First i just want to always stress that i don't want to do anything that would add a lot of long term stress for you. The thought was that if it was slightly more module it might make it easier to manage. But like you said your the person who wrights 90% of the code so if you find it harder then it will slow down the majority of the work being done. Which will hurt the project at large.
As for the idea of the time put into this. Don't worry to much about that. I've learned a lot and i'm fine working on my repo. This is just my test bed at the moment, so it's fairly ruff now so maybe someday it will look better and make more sense.
Like i said i'll make a larger post after work. I just figured you were gracious to give a very well thought out response so i thought it deserved a quick response.
-
So I'll answer the questions below on how it works and why. But i don't think that's whats actually the matter here. So I'm just going to talk about what I'm thinking about doing.
What I'm thinking of doing is to continue to work causally on this on the side, but I'm going to stop messaging you and this forum about this because its out of scope of this project. I understand that this is something that is kind of a pet project of mine and will in almost no way make the product you make any better.
In the very, very unlikely scenario that many people end up finding my changes helpful. Then we can talk about the potential nightmare of merging it to the main repo. But at the moment theirs no need or huge interest in these specific changes. So at the moment at don't think we need to talk about this any more.
Hopefully this is a no hard feeling on both ends. I still love this project and will try to merge any general Espruino bug fix i find.
-
Goal
So just to make sure the goal of my intentions are clear. I never tried to merge this code into the repo because i knew it wasn't good enough to go into the main project. The goal was just to see if this was something worth doing.LCD_BL
So that's a bug from my lack of knowledge on the "LPM013M126". There's a lcdMemLCD_extcominBacklight and it has a BL pin. So in theory the BL pin should just be in the bangle_backlight_impl.c for hardware that is generic across devices.LPM013M126
Yes this is for the bangle device. I wanted all the files to mostly be hardware and not device specific. I figured anybody going to try and work on the bangle.js c code, will figure out what hardware is in it.7 files for backlight
So i can break down the numbers- 2 files for jswrap_bangle_hardware.c/h - these hold the JavaScript functions and no hardware specific stuff.
- 2 files for bangle_hardware.c/h - holds generic functions that can be overwritten by device specific calls.
- X files- X being the number unique pieces of hardware that need functions being overwritten
So for some of these the override files are extremely small and look like a waste of time, but even for just lighting the F18 and the fade implementation both use more then 60 lines each and the code for them was not just in one place but all spread through the project. So for me i can navigate this structure a little better. But i do agree this add a lot of files, but the complexity of the device won't make the number of files go up you'll just have more functions in the file. You can see this in the display vs backlight driver. Same amount of files, but the display driver has way more in it.
finding all the function github is a nightmare
Yes, i would 100% agree. Nightmare is probably underselling that issue. My software style is to have code very spread out. I like my code to fit under that 100 line limit so i don't need to scroll to much. This is probably do to my setup. I personally use like 2 large monitors or up to 4 small monitors at any given time. So i use multiple instances of visual studio code. So i me its completely normal to trace through functions calls.Code Flow
My goal would be such that jswrap_bangle.c would only hold basic states of the watch. So a startup,events,shutdown. Then the rest would be in the hardware folder.Final Note
So these are just my thoughts to hopefully clear up some of the topics above. This specific post doesn't need a response if you don't want too. I think i understand were your coming from. Well at the minimum trying my best to.This took me a while to wright up. So I'll post the WHY I'm doing this in a little bit.
- 2 files for jswrap_bangle_hardware.c/h - these hold the JavaScript functions and no hardware specific stuff.
-
Personal Reason
So you asked this I don't really understand why we're doing this, we could support the P8 with just another few IFDEFs.
So this isn't really about the P8 watch per say. This is actually about a different project in mind.Project idea
I'm one of those weird people who loved the pebble watch, but the device is just so old it's hard to use now. So what i started looking into was how to replace the motherboard and what software should it run. Out of all open source watch "operating system" i found that yours seemed to make the most sense. Mostly because it allowed apps to be loaded from a web browser, which no other watch OS can do.The problem is I'm planning on using hardware that is very different then what most people are using. I'm using a Nrf52480 and the memory LCD, but everything else is fairly different. So here comes the problem, i didn't want to pollute the main project with a lot of #ifdefs that were specific to my small project. So i was hoping I could break the code into modules then i could easily have my own repo with my own drivers that i could easily update down the line.
So that's where the P8 comes in. I thought trying to add the P8 into the code base would be a good test project to see how hard of a task it would be. Its also a watch i had lying around and stopped using it because it was very hard to develop apps for compared to the bangle watch.
As to why i didn't say all this stuff in the beginning. It mostly comes down to not knowing how serious i was about this project. This kind of project is a lot larger then any other project I've ever tried to do. So my thought was to see how hard it was to do the P8 watch and then see if i want to continue and try to make my own PCB for the pebble watch. My hope was either way i thought it could potentially help the project out.
So i hope this makes more sense and I'm sorry if i pressed to hard.
-
What I'm thinking of doing is to continue to work causally on this on the side, but I'm going to stop messaging you and this forum about this because its out of scope of this project.
You can still post to this forum topic no harm done, it is not out scope.
If it would work I would try to add couple of watches to this.
Even if this would be maintained in a fork then it is still interesting enough to post some updates or ideas here.As for the code style/complexity that is probably the main issue here I did not check it yet in detail.
And BTW there is still also the javascript way.
-
It took some time to sort out my thoughts about modularization.
This is how I started. Jswrap_bangle.c was hard to understand because of the ifdefs. And even the small bootloader main.c code was hard to read and I think there might be some ifdef combinations that won't work together. Finally I implemented my code only for the bangle js 2. That makes it easier to test and less risky but the other watches don't benefit.
Is all this really a problem? I think it depends on the goal for your OS (and especially the bangle code). Do you want it to be an OS ike linux to be run on am almost unlimited range of hardware? This would mean a lot of work to add a hardware abstraction to the bangle code and who would use it because most watches, etc are closed for custom firmware.
So I think it is more attractive to make the bangle code for your own limited set of watches with it's own ecosystem and apps.
Thinking about a bangle js 3 there could still be some improvements like less ifdef nesting or even everything related to one device in one file instead of supporting 3 (and then 4) devices in one jswrap_bangle.c file.
There is a lot of potential for the bangle ecosystem to focuse on like more UI abstraction so apps once developed run on different devices, integrated companion app extensions like e.g. the zepp os architecture (maybe gadgetbridge could be the host) and of course a new bangle js 3 hardware. Maybe api levels and storage seperation.
-
We could support the P8 with just another few IFDEFs in jswrap_bangle.c and it wouldn't make it noticeably harder to understand than it is right now.
To make it worthwhile it is definitely not just about P8, and with each other watch the complexity grows (and sadly not in linear way). As for "wouldn't make it noticeably harder to understand than it is right now", well yes that is true because it is already too hard to understand so one more ifdef won't change anything :-)
~/Espruino$ grep ifdef targets/nrf5x/bluetooth.c | wc -l 49 ~/Espruino$ grep ifdef targets/nrf5x/jshardware.c | wc -l 82 ~/Espruino$ grep ifdef libs/banglejs/jswrap_bangle.c | wc -l 312I find the nrf5x bluetooth support quite complex already - it supports nrf51,52, softdevices version 2,3,4,5,6
Anyway I understand that it works for you and gets the job done so it is hard to argue about that.
I checked the https://github.com/brendena/Espruino/blob/MOD_DISPLAY/libs/banglejs
and so far I only find the _impl stuff quite odd. there is jswrap_xx for the JavaScript binding layer and the rest in .c files is of course implementation that is what the .c files are for, so I don't get why there is _impl everywhere. Will look more into how it actually works.Even splitting the big file into more smaller ones (still with all the ifdef spaghetti) could be useful. something like jswrap_banglejs_display|touch|accel|hr|compass|gps|UI|... so that it s not so huge, the main file would just have the rest that does not belong anywhere special or is common to more
-
Thanks everyone! Sorry for the delay replying - thanks for the really detailed responses @user156811!
Absolutely no hard feeling from my side at all - I really appreciate what you're attempting, I just feel bad I'm not so convinced by what's there now :)
You'd mentioned splitting out the barometer code earlier. Did you ever look at that? I think despite it seeming easy, perhaps the backlight code is integrated into enough stuff that actually splitting it out nicely ends up being problematic, and the barometer might be more straightforward.
I like my code to fit under that 100 line limit so i don't need to scroll to much
Do you mean the files themselves, or just functions?
For me VS Code's Ctrl-click -> definition doesn't work very reliably outside of the current file (possibly because of all the ifdefs! Eclipse didn't handle it well either), and so when I end up drilling down multiple function calls which are in different files, it makes it a bit more tricky to follow through smoothly.
There's probably a good middle ground between all-out abstraction and the current insanity that is
jswrap_bangle.cthoughDo you want it to be an OS like linux to be run on am almost unlimited range of hardware?
It's a good point - and I think realistically it's somewhere inbetween. Ideally as flexible as we can get without introducing much extra code/memory usage because of the abstraction.
Some of the nRF52 builds are very tight - we're now at a point where even something like this fix pushes the code size up enough that the firmware doesn't fit on Puck.js devices without changes!
Another consideration is it's not just Bangle.js that has sensors (Puck.js/Microbit/etc have them too) and in an ideal world we'd be able to reduce duplication there too - but I think possibly that one's just a step too far for the moment.
splitting the big file into more smaller ones (still with all the ifdef spaghetti) could be useful. something like jswrap_banglejs_display|touch|accel|hr|compass|gps|UI|... so that it s not so hug
Yes, I think that could be a good call - maybe a good starting point on the road to making things easier for contributors?
it is already too hard to understand so one more ifdef won't change anything
exactly :)
-
I find the nrf5x bluetooth support quite complex already - it supports nrf51,52, softdevices version 2,3,4,5,6
And yes, I hate that too - it's often hard to see a clean way around it though without adding a bunch of duplication, which has its own challenges!
It's also sometimes hard to justify pouring work into making things better - the SDK11 port is maintained basically just for the Microbit 1 (nRF51) but apart from @fanoush I'm not really aware of anyone using it!
-
So i did break out the barometer. I also broke out the accel,compass and touch. I did this fairly quick, so there might be parts that need to be moved over. I plan to do a second/third pass through it when moved the easy parts over. I put it on a new branch called quick_rewrite
So moving the barometer took me a little bit because i got confused on how the SPL0 worked across versions. So you can see the barometer code here
- barometer libs - supporting libs
- SPL0 driver
- barometer jswrap file
- barometer impl - Holds all the functions that can be overwritten and hold many of the states. So idle,init,on,off
- Bangle 2 device defines
Another consideration is it's not just Bangle.js that has sensors (Puck.js/Microbit/etc have them too) and in an ideal world we'd be able to reduce duplication there too - but I think possibly that one's just a step too far for the moment.
I think this would be awesome! I didn't dare go down that route because i know i would break something. Sounds like a fun thing to experiment with a new piece of hardware and then maybe back port some of the idea back to it.
There's probably a good middle ground between all-out abstraction and the current insanity that is jswrap_bangle.c though.
So i just found that there's all these "middle ground file" in the misc. For things like heartrate and gps. I just started to look at these things next. So they might be a middle ground somewhere there.
- barometer libs - supporting libs
-
Comment/Question
_impl stuff quite odd
so I don't get why there is _impl everywhereI can see that. I don't think this is a standard way of doing things. This is kind of my hack to get the code moved out of the main bangle file without adding any runtime abstraction. Most abstractions like how linux kernel would add a lot of overhead but this should produce basically the same or close to the same binary.
How it works
So the impl name scheme is like this
- Bangle_hardware_impl.h/c - "Impl" short for implementation
So if you think of this as like OOP. This file is your base class. Then i label most of the functions weak to make them "virtual" functions.
- Bangle_ hardware_device_impl.c
Then you can make as many device files as you want. Any function in these files will override the functions in the base_impl.c file. In these files you can override all or just some of the functions. Many pieces of hardware don't need a completely setup. So when it builds what ever functions it doesn't override will you the base_impl.c implementation. Which in many cases is just nothing. Also these files are added at compile time and are chosen based on the hardware defines in your device.py file.
example
This is the file for the KX023 accelerometer driver.
answer
So basically there's a lot of .c files because i use a different c file for each configuration needed for each piece of hardware or feature.
- Bangle_hardware_impl.h/c - "Impl" short for implementation
-
Thanks! Yes, that looks really good, and it's nice clean separation.
I wonder whether for now, we even need
libs/banglejs/hardware/barometer/bangle_barometer_impl.cand the ESPR_WEAK functions - I feel like we could probably just have that injswrap_bangle_barometer.c. I feel happier in a way if we just define all the functions in every hardware implementation file, and the compile fails if eg someone missesbanglejs_barometer_off_impl.I like pulling out the repeated I2C initialise calls to arrays (did you ever see how much flash it saves?).
-
Thoughts on just jswrap_bangle_hardware.c
So i think that would be a huge step forward. My biggest complaint was not knowing where to edit the code if i wanted to add something. So if you split out the each piece of hardware out then it make it easy to know where to go if you want to add a IMU or something. Which is huge!
So if you don't like the idea of WEAK functions, but do like the idea of splitting the files out. We could just have a bangle_HARDWARE_impl.h file and no *.c file. So then every HARDWARE_impl.c file has to include every function. That would accomplish the same thing. I think i remember us talking about something like this before.
So i personally like the idea of splitting out the drivers into there own files because then it's easy to find all the code thats specific to that given driver, but also because it hides some of the weird parts of other drivers. For me one of the weird things is seeing all the things special to just the BANGLE 1. From a hardware perspective it kind of just does things different then most of the other watches. So moving all there hardware specific stuff to a separate file makes the rest of the code easier to read.
Final thoughts
So i think moving all the code into specific JSwrap files is a great step forward. Its exactly what i did when making all my changes.I2C calls
I2C array calls. I don't know how much flash it saves if any, but i do think it reads better.Ex:
- Write(IMU, IntI2cCallsAccel); //pseudocode code example
It's kind of self documenting code.
@Gordon - Write(IMU, IntI2cCallsAccel); //pseudocode code example
fanoush
Chriz
Gordon-
Post a reply
About
modularizing BangleJS
Posted by
@user156811
I REALLY NEED TO HEAR OTHER PEOPLE'S THOUGHTS ON THIS...
@user158096 and @fanoush I think you're the only ones that have expressed interest in this so far? Please can you have a look at https://github.com/brendena/Espruino/tree/MOD_DISPLAY/libs/banglejs/hardware/backlight where the display backlight code has been split out, and imagine that there's an issue setting the backlight brightness on the Bangle.js 2 with
setLCDPowerBacklight(or anything else, like maybe adding a new watch), and let me know, honestly, whether you think it's easier to find/change the code in the refactored repo, or the old one?Being totally honest, every time I see a message about these changes, it just fills me with dread... I thought I was just being averse to change and not giving it a chance, so I thought I'd give it a go to see if it really was easier to work on than the current code.
So... I started looking, trying the above. I found jswrap_banglejs_setLCDBrightness in one file (which is fine - that would automatically link from the reference pages anyway) , and that calls jswrap_banglejs_setLCDPowerBacklight, which calls banglejs_setLCDPowerBacklight_impl (which isn't documented) and that calls banglejs_pwrBacklight_impl (which isn't documented either) sometimes, but I'm not sure for what boards it does that or which file is for which device, and banglejs_pwrBacklight_impl is empty. I do see
banglejs_setLCDPowerBacklight_implin bangle_backlight_f18_impl.c, and that does set the brightness, but for Bangle.js 1. Andbacklight_fadesets it too.banglejs_setLCDPowerBacklight_implinjswrap_bangle_backlight.cdoes have a#ifdef LCD_BLthat sets the brightness, but it's also set inbangle_backlight_impl.cas well (actually I think that was only in the MOD_BACKLIGHT branch which I realise now is older), and the pin is set inbangle_backlight_LPM013M126_impl.cwhich I know is also Bangle.js 2 but isn't obvious - so I'm not sure which files actually do Bangle.js 2, but I have a sneaking suspicion that LCD_BL is now set from at least 2 separate files even on the latest branch (3 if using lcd_fade which we don't currently).And this is just for the backlight, that's a glorified LED. It's now implemented over 7 source files. I don't know what it'll look like for something more complex.
Trying to find all those functions in different files on GitHub is a nightmare, and I guess it only really works if you've got VS Code and you can do a text search over the whole project for everything. I'm fully aware
jswrap_bangle.cis big and people don't like it. It's a mess of legacy code and untangling it is a huge problem, but I'm not convinced taking that code, adding more indirection, and splitting it over multiple files is going to be better even if it does reduce the size ofjswrap_bangle.c.I've spent hours this morning trying to go through this, and be honest with myself about it, but it's just doing my head in...
I know you've spent ages on this, but I'm here wondering how on earth I'm going to maintain anything after these changes go in. Realistically I end up doing at least 90% of the work on this, and I feel like it's going to make my life much harder... not to mention something like the DICKENS/Starfield watch which was a project for a customer and which I think is very unlikely to work afterwards without a lot of unpaid work from me - I'll just have to take that out of the build and leave it as an unmaintained fork.
... and I don't really understand why we're doing this. We could support the P8 with just another few IFDEFs in
jswrap_bangle.cand it wouldn't make it noticeably harder to understand than it is right now. Nobody's even asked me about PineTime/P8 in the last year anyway, and very few people contribute or have expressed interest in contributing to the main Bangle.js codebase.I feel like with the best will in the world, this is going to take weeks of my time vetting/testing and fixing regressions, it's going to add bugs for over 10,000 paying customers, and all to make it marginally easier for one of two people to use it (maybe) on a $25 watch. None of this will benefit me or 90% of actual users.
I'm sorry, but I feel like I have to be honest about my feelings on this.
I know you've put bags of work into this but I also don't want you to put in loads more if I can't bring myself to merge the changes in - and I'm looking at https://github.com/brendena/Espruino/tree/MOD_DISPLAY and just can't see how merging it or anything like it isn't going to cause me lots of grief.
At the end of the day, I'm trying to make a living on software I'm giving away, and I feel like most days it's a massive struggle. The vast majority of work I do I don't ever get paid for - and I don't really have the luxury of just changing stuff for the sake of changing it, because with the best will in the world there will be issues which need sorting out, and at the end of the day it'll be me that has to deal with them...