You are reading a single comment by @Gordon and its replies. Click here to read the full conversation.
  • 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...


Avatar for Gordon @Gordon started