modularizing BangleJS

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

• I don't have either a bangle 1 or 2. I plan to buy one just currently it would be a pain to ship it to were I'm at.
  
• this code isn't cleaned up at all. Just something I'm playing with.
  
  

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.

I got a small video showing the current state.
https://youtu.be/GTdgfV9q_DU?si=oRSj-GhJ­iGByzBRH

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 https:// discord.gg/7vxUDFJN but it is pretty quiet nowadays and most activity is in the epaper/pricetag channels. that's where porting stuff to various nrf52 watches was discussed but nowadays there are not much news as most manufacturers moved to other chips and older watches are harder to get.

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:

• You mentioned in your video about the EMULATED defines - this would actually be a great opportunity to get all those removed. The emulated Bangle.js 1/2 devices could just have their own implementations
  
• WRAPPERSOURCES is for files that get parsed for /*JSON ... headings - so if you just want files added to the normal build like bangle_lcd_generic_display.c, you just want to add them to SOURCES
  
• Similarly every other file with r /*JSON ... headings starts jswrap_....c and it'd be nice to continue that eg to make bangle_lcd.c into jswrap_bangle_lcd.c
  
• I think function naming could do with a bit more thought - why are functions prefixed setDefault? For instance setDefaultPowerController isn't obvious that it's actually setting LCD power, and if we're using weak functions, the function will always be called 'default' regardless of whether it is the default or not... Maybe if we have a function like jswrap_banglejs_lcdWr than calls into another function to access the hardware, we should just name that function jswrap_banglejs_lcdWr_impl or something, so at least it's really obvious when we see jswrap_banglejs_lcdWr_impl that it's called from jswrap_banglejs_lcdWr?
  
  

Anyway, I hope that's some help...

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.

I was looking at it as a proof of concept ... deciding what ... should be done differently.

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!

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.

Ok, great! That sounds really promising

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 folder

So 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.

• jswrap_bangle_xxxx - hold all the jswrapper function
  
• banglexxxximpl - will hold the generic implementation. So many function with no definitions
  
• banglexxxxhandware_name_impl - the device specific funtions
  
  

Devices folder

Holds defines and will hold probably some simple setup function.

bangle_defines.h

Which holds all the enums from jswrap_bangle.c
It holds references to the global state of bangle. These are mostly timers and bangleState and flag.

This sounds ok - but maybe if bangle_defines.h is going to have all the state, it should be bangle_state.h?

I'm also not sure if we really need empty weak functions? If the function is going to be required for the given hardware to work, maybe just put the declarations in the header? If the function is needed, having the build fail if the function isn't defined is probably a good thing?

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.

• this partly has to be like this. Your soft reference function have to compiled separately from your hard reference function.
  
  

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.

So those week functions are to handle state.
So in the code i just sent the Bangle1 watch needs its backlight to be killed a certain way. But most backlights don't need to be killed at all.

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.

• idle
  
• kill
  
• init
  
  

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.

If the function is needed, having the build fail if the function isn't defined is probably a good thing?

So i totally agree with this. Its just in these cases these function are not needed in many cases.

CHANGES

• build EMSCPRTION
  
• moved state to a separate file called bangle_state.h
  
• removed most of the EMULATED defines
  
  

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

• So I'm able to build the emscription repo. I just don't see it when run the repo? Currently i won't be able to test with real hardware till next month.

• So other then testing i can start working on the display module. Or try to address other concerns that you have.

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

a big change that will affect anyone who works on the Bangle.js codebase,

Even me who mostly stay in the espruino/BangleApps repo?

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.

fanoush looking at it

was away last week, will hopefully check it this week

Look forward to what you have to say.

So i added the display driver layout as well.
https://github.com/brendena/Espruino/tre­e/MOD_DISPLAY

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.

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