modularizing BangleJS #6875
Replies: 1 comment
-
Posted at 2024-02-20 by user156811 I got a small video showing the current state. Posted at 2024-02-20 by @fanoush This is great effort and overall it looks good, thank you, please continue :-) BTW there is also discord with espruino (and arduino and a bit of pinetime and waspos) smartwatch channels here Posted at 2024-02-20 by @gfwilliams Hi - sorry for not getting back to you sooner, I was away last week. Thanks for looking into this - it's a huge job. I'd be interested to hear others' thoughts on this... jswrap_bangle.c does look cleaner but I'm also aware that basically all the hardware specific code like peripheralPollHandler has just been deleted, so it would be interesting to see how everything looks when it's all working... I'm a bit anxious about your approach of deleting all the code to get a minimal build and then trying to re-add it after. I feel like it is going to pretty much ensure that Bangle.js 1 and 2 end up getting broken (or at least changed so they behave differently to how they did before), and at that point it will be impossible to track down at what point it happened and fix it easily. It feels like most code was ifdef'd anyway, so you could have got a working build for P8 reasonably quickly without deleting it, and then at that point you could have started moving code around one peripheral at a time - so at least if something did break we could find the git commit where it broke and understand what happened. I just feel like this is going to be a lot of work, and I want to be able to pull it back into the main repo - but in all honestly if it turns out that Bangle.js 1 and 2 end up broken in lots of ways after, it's going to be very hard to me to justify merging it back if it's going to mean months of me fielding complaints and fixing issues, and a really bad experience for the thousands of Bangle.js users out there. Just a few things on this:
Anyway, I hope that's some help... Posted at 2024-02-20 by @fanoush I was looking at it as a proof of concept how it could be done so if it works at least for P8 and some percentage of bangle apps work on top of it then the POC is done and we can learn from it how it can be done properly for real - still reshuffling and renaming stuff or even deciding what went completely wrong and should be done differently. Posted at 2024-02-20 by @gfwilliams
Yes, absolutely - I'm totally behind that. I just don't want @user156811 to spend ages working on this particular port, and to have something that works great for P8 but that can't be merged upstream because it would break too much. I guess in an ideal world, we'd have a bunch of smaller, manageable PRs that slowly untangled things a bit at a time (rather than one massive thing), and I'd feel a lot more comfortable about that! Posted at 2024-02-20 by user156811 Thanks for the feedback :) Making a bunch of small PRs makes a lot of sense to me. It avoids doing a really hard complete test at the end and avoids the problem of having to bring up my branch to the current espruino branch. I removed most of the code in my build because i thought it would be easier to understand the flow of the code. I wasn't all that worried about merging it into the main repo because i just wanted to properly convey what i was thinking. Then we could figure out what to do from there if it was worth the effort. I think it should be "fairly easy" to do this in smallish PRs. Since i didn't change how things worked. The only things that changed would be the functions that your moving out. So each PRs should be fairly self contained. Posted at 2024-02-22 by @gfwilliams Ok, great! That sounds really promising Posted at 2024-02-23 by user156811 So here is where i'm housing the code i'm working on. I haven't fully tested this but it shouldn't be far off from what I'm expecting. Hardware drivers folderSo this branch is just working on the backlight. One thing you'll notice is i had to move around a decent amount of code. This is because there's so much stuff that's defined inside the "jswrap_bangle.c". So i'm planning on there being 3 types of files.
Devices folderHolds defines and will hold probably some simple setup function. bangle_defines.hWhich holds all the enums from jswrap_bangle.c Posted at 2024-02-23 by @gfwilliams This sounds ok - but maybe if I'm also not sure if we really need empty Posted at 2024-02-23 by user156811 Now currently the build is a little ("lot bad"). Partly because there will be a huge influx of .C files that i didn't want to have to list them all. So currently i have a lot of #includes(****.c**) files. This will include all the files, but each file will be covered with define to enable or disable it. In the board file i need to add a line to grab all the hardware specific defines.
Then this grabs all the hardware implemenation files. I have a #include(**.c) as well in the jswrap_bangle.c. I'm guessing there's probably a way to handle this in the makefile. Just haven't looked into it yet. Posted at 2024-02-23 by user156811 So those week functions are to handle state. There's also a few display drawing calls that are not really supported on other devices other then the bangle1. If i get to do touch. The bangle1 touch needs no initialization. But the bangle2 does. There are other examples. I can point out where a specific devices doesn't need a special state, but the group as a hole needs the option to config at a given state.
So these empty declarations are there because its not needed for every devices. So each device could have every function declarations inside of them. But they would also just be empty.
So i totally agree with this. Its just in these cases these function are not needed in many cases. Posted at 2024-03-04 by user156811 CHANGES
So to remove most of the EMULATED define i needed all the functions in the XX_impl.h header to be defined just not doing anything. So i just wrap each function definition in a macro that will generate a empty function. Example header how how it used. Then with the EMSCRIPT builds i just don't include the XXX_impl.c files. Status
Posted at 2024-03-04 by @gfwilliams Thanks! Yes, this looks good - although I guess if it were possible to just treat the Emscripten build just like another type of hardware (eg a _impl file with empty functions) I think that might be preferable? After all the point of this is really to try and avoid special-case stuff as much as possible. It'd be great if others could take a look at this and let me know what they think though Given this is going to end up being a big change that will affect anyone who works on the Bangle.js codebase, it would be good to have more than just me and @fanoush looking at it Posted at 2024-03-04 by @thyttan
Even me who mostly stay in the espruino/BangleApps repo? Posted at 2024-03-04 by user156811 As long as it doesn't add any new bugs it shouldn't effect the BangleApps repo. The code suggestion i'm putting forward are to basically separate out the hardware into there own modules in the bangleJS library. Then from there separate out how each piece of hardware works. Posted at 2024-03-11 by @fanoush
was away last week, will hopefully check it this week Posted at 2024-03-12 by user156811 Look forward to what you have to say. So i added the display driver layout as well. Posted at 2024-03-12 by user156811 One thing i wanted but couldn't figure out was a way to have just a Bangle_driver_impl.h without a Bangle_driver_impl.C I wanted this because the file mainly includes empty weak functions file because the whole point is to give the project a place to inject hardware specific code. But because the each bangle_device_driver_impl.c includes the headers of the Bangle_driver_impl.h you can't have the weak pointer definition because then you'll get double declarations error. Posted at 2024-03-15 by @gfwilliams Yes... I think if you're defining something weak you have to have it in a C file. I guess that's not the end of the world though Posted at 2024-06-02 by user156811 So i got the BangleJS2 watch working just like the normal build. Now trying to get the bangleJS1 working properly. It draws properly until it reads from the flash and then it just draws wrong. Then when you connect it to the Web ide it still draws weird. Even when you tell it do draw direct. But I'll figure that all out soon. Its just been a few month since I've updated this. Mostly been working on other hardware and my job's been taking up too much time lately. haha Posted at 2024-06-04 by @gfwilliams That's great! Not sure what could be causing the Bangle.js 1 issue, but hopefully you'll find it soon :) Posted at 2024-06-04 by Chriz Great that you are working on the modularization and the jswrap_bangle.c is already smaller. I am trying to wrap my head around the different cases of button presses that trigger reboots, recovery menus, etc and all the defines make it really hard to read. So I hope this will get a bit better, too. Posted at 2024-06-07 by user156811 Ok, so I've checkout a version from February that had no modifications and it also doesn't work. So I'm guessing it has to do something with "DESPR_PACKED_SYMPTR". I'm guessing something I've flashed onto it is not compatible to the newer stuff. Posted at 2024-06-09 by user156811 Yep, i merged up with the current build and it fixed all my problem. So It currently has backlight and display code moved to separate drivers. Posted at 2024-06-17 by @gfwilliams 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 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
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 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 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... Posted at 2024-06-17 by user156811 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. Posted at 2024-06-17 by user156811 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. Posted at 2024-06-18 by user156811 Goal LCD_BL LPM013M126 7 files for backlight
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 Code Flow Final Note This took me a while to wright up. So I'll post the WHY I'm doing this in a little bit. Posted at 2024-06-18 by user156811 Personal Reason Project idea 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. Posted at 2024-06-18 by @fanoush
You can still post to this forum topic no harm done, it is not out scope. 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. Posted at 2024-06-18 by Chriz 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. Posted at 2024-06-18 by @fanoush
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 :-)
I 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 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 Posted at 2024-06-24 by @gfwilliams 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.
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
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.
Yes, I think that could be a good call - maybe a good starting point on the road to making things easier for contributors?
exactly :) Posted at 2024-06-24 by @gfwilliams
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! Posted at 2024-06-25 by user156811 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
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.
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. Posted at 2024-06-25 by user156811 Comment/Question
I 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 worksSo the impl name scheme is like this
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.
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. exampleThis is the file for the KX023 accelerometer driver. answerSo 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. Posted at 2024-06-28 by @gfwilliams Thanks! Yes, that looks really good, and it's nice clean separation. I wonder whether for now, we even need I like pulling out the repeated I2C initialise calls to arrays (did you ever see how much flash it saves?). Posted at 2024-06-29 by user156811 Thoughts on just jswrap_bangle_hardware.cSo 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 I2C calls Ex:
It's kind of self documenting code. |
Beta Was this translation helpful? Give feedback.
-
Posted at 2024-02-12 by user156811
So i'm working on making BangleJS work with my Pinetime and to do that I'm trying to migrate some code outside of the main jswrap_bangle.c file. So it will be easier to add the PineTime specific code.
So the code here
Notes
My primary goal is to move out to its own folder. Then have function that can be overwritten based on what piece of hardware you using. There's probably a more advance way of doing this, but it seemed to clean it up enough that i could follow the logic of what bangle was doing and how the hardware did it.
Currently i only have Backlight and display. But it shouldn't be too hard to continue this pattern for the rest of hardware. There even might be a nice way to wrap things like GPS,HRM,Compass since they follow a similar pattern of ("is ON", set power, rd,wr). So then you should have a array of hardware to iterate through.
So this is a continue from this post. I'm posting this to see if this is something that should be continued or if its making some compromise that makes the the situation worse.
Beta Was this translation helpful? Give feedback.
All reactions