QMC5883L support for hmc5883l.c module (#2187) #2188

infrat · 2017-11-29T21:20:25Z

Fixes #2187 .

This PR is for the dev branch rather than for master.
This PR is compliant with the other contributing guidelines as well (if not, please describe why).
I have thoroughly tested my contribution.
The code changes are reflected in the documentation at docs/en/*.

QMC5883L chip support introduced to hmc5883l.c library. Unfortunately, I don't have magnetometer based on HMC5883L chip to test it. My changes are tested ONLY with QMC5883L!

pjsg · 2017-11-29T21:59:55Z

app/modules/hmc5883l.c

    uint32_t sda;
    uint32_t scl;

-    platform_print_deprecation_note("hmc5883l.init() is replaced by hmc5883l.setup()", "in the next version");
+    platform_print_deprecation_note("hmc5883l.init() is replacd by hmc5883l.setup()", "in the next version");


This introduces a spelling mistake.

pjsg · 2017-11-29T22:02:22Z

app/modules/hmc5883l.c

@@ -106,11 +176,22 @@ static int hmc5883_read(lua_State* L) {
    return 3;
 }

+static int xmc5883_read(lua_State* L) {
+    uint8_t i2c_addr;
+    i2c_addr = xmc5883_identify();


This introduces a delay on each call of read(). Why not save the i2c_addr in xmc5883_i2c_addr (as part of setup/init) and then just access the right address immediately.

I was considering approach you suggested, but I wasn't really sure if nodemcu modules stores their states between each LUA command call - that's why I decided to determine chip type each time. But if it's possible to store such value, then I'll do it the way you suggest.

infrat · 2017-11-30T22:31:59Z

Changes commited

devsaurus · 2017-12-17T20:39:29Z

I'd merge this one if there's confirmation that HMC5883L isn't broken. @pjsg maybe?

infrat · 2017-12-17T21:15:05Z

@devsaurus hold on for a moment, please. Few days ago I've found a minor issue with my change, when used with HMC/QMC disconnected from I2C bus. By default in such case it should throw error luaL_error(L, "device not found") but it doesn't happen (I suspect it's because module identification done here: https://github.com/infrat/nodemcu-firmware/blob/ff7bb2d525815ab65a56b03ec19776a3387f12d8/app/modules/hmc5883l.c#L55 )
After Christmas I should prepare fix for that.

It doesn't affect the library in way making it unusable, but it's a bug and I should fix it.

pjsg · 2017-12-18T14:46:47Z

I don't have either device....

edtamarin · 2017-12-22T18:18:26Z

I have used the HMC library from the latest master build successfully, and the "device not connected" error is thrown when the HMC is not on the bus. I have not tested this particular HMC build.

However, I had some additions to the module in mind, since, at least for the HMC chip, I think the module is too simple and can be made much better. I have opened #2206 for that and planned to make some changes over the Christmas holidays myself.

My idea is that for the HMC, and really for any compass module, it is good to have the library output the heading value, since the NodeMCU does not have built-in trigonometry and have more configuration options than just reading the raw data with some given presets.

Unfortunately, I only have the HMC chip so I can add features only for that at this time. I would like to hear your thoughts on this.

marcelstoer · 2018-01-08T21:11:45Z

A fundamental problem is that the module name is equal to the chip name. Hence, someone looking for support for QMC5883L would be disappointed because he/she would only see HMC5883L in the modules list. The same applies for search engines of course.

We can change the title on line 1 in hmc5883l.md and we can change the navigation item name to something like "hmc5883l / qmc5883l" (I suggest we do both) but you would still say hmc5883l.read() even if you run it against a QMC5883L. I'm not sure if we just have to live with that.

infrat · 2018-01-08T21:25:28Z

Yeah, I agree with you but in fact it's quite hard to deal with that. For the moment, I see two ways: we can do it as is done in this PR, or create separate library for QMC. The question is which is better? Even I can't say which is better to me. Both solutions has pros and cons.

marcelstoer

Change title in documentation and navigation item name as proposed at #2188 (comment)

devsaurus · 2018-01-09T19:36:13Z

I studied the datasheets of the two chips and understand the headaches here better now. Both only share the same model number 5883 and the fact that they're magnetometers. Apart from these, register interface and operation modes differ. Initially I though that QMC5883 would be the bigger/newer/extended version of HMC5883 but no - they're different architectures IMO.

For the moment, I see two ways: we can do it as is done in this PR

I would avoid that for the reasons above.

or create separate library for QMC

Not my favorite solution but provides a better separation.

Third alternative (same topic as #1242 (comment)): Remove the chip-specific module and integrate HMC5883 and QMC5883 (and future ones) in a magnetometer module. Users derive objects from this class and run the setup() and read() methods on the object

hmc = magnetometer.hmc5883l(i2c_id)
newconfig = hmc:setup({MS=0, DO=4, MA=3, GN=1, MD=1}) -- normal measurement, 15Hz, 8 avg, single-shot mode
-- wait some time or wait for GPIO trigger by DRDY
x, y, z = hmc:read()

qmc = magnetometer.qmc5883l(i2c_id)

Implementations on C level would be quite specific to the chip type, same for the setup() arguments.

Pros

Single magnetometer module (or any other swift name)
Clear separation, no need to support multiple architectures with one piece of code
Extensible with further chip types
Enables consistent module doc

Cons

Additional command to derive an object
RAM usage for Lua object
All-or-nothing option, qmc5883 routines use flash memory even if you only need hmc eg.
Increased complexity on C level (slightly, IMO)

What do you think - would this third option be a feasible way out?

infrat · 2018-01-09T20:19:17Z

@devsaurus if you compare block diagrams of both chips - they're very similar. From architectural point of view there are not too much differences between them. They have different registers addressing, and probably clock is little bit different too. Sad thing is that many manufacturers use QMC and HMC interchangeably what makes customers little bit confused (like in my case).

Regarding module code, third option you suggested I like the most. It seems to be clear enough and straightforward. Do we have any modules using such architectural approach?

devsaurus · 2018-01-09T21:02:30Z

Regarding module code, third option you suggested I like the most. It seems to be clear enough and straightforward. Do we have any modules using such architectural approach?

u8g and ucg are the only ones if I remember correctly, but they are quite complex to serve as a blueprint. Given there's consensus to head into this direction (@marcelstoer, @pjsg?) then I'd volunteer to set up such a module including current hmc5883l code. I don't intend to offload this maintenance effort to you 😃.

we're heading into a different (better) direction

infrat · 2018-01-10T19:39:48Z

we're heading into a different (better) direction

OFC direction! because it's a magnetometer ;)

@devsaurus I think I can try to setup both modules in the way you suggested, so don't worry. I'd be really happy to have some influence on this project. At least, if I wouldn't manage I'll ask you for some help.
In this moment I'm focused on other ESP-related project, but hopefully I'll be able to find some time to do this soon. Just give me few weeks. I hope it's not really urgent?

marcelstoer · 2018-01-10T19:49:22Z

Ok, then let's close this PR and either @infrat or @devsaurus will create a new one.

devsaurus · 2018-01-10T20:58:39Z

I think I can try to setup both modules in the way you suggested

Thanks you very for your support! It's highly appreciated.

It's not urgent at all from my point of view. Feel free to come up even with a prototype if you look for early feedback.

pjsg reviewed Nov 29, 2017

View reviewed changes

infrat force-pushed the qmc5883l-support branch from 4a97b82 to 220d07d Compare November 30, 2017 22:27

QMC5883L support for hmc5883l.c module (nodemcu#2187)

ff7bb2d

infrat force-pushed the qmc5883l-support branch from 220d07d to ff7bb2d Compare November 30, 2017 22:29

devsaurus added the new feature label Dec 17, 2017

marcelstoer mentioned this pull request Dec 22, 2017

HMC5883L settings support & changing data output formats #2206

Closed

marcelstoer mentioned this pull request Jan 8, 2018

support for ads1015 #2223

Closed

marcelstoer previously requested changes Jan 9, 2018

View reviewed changes

marcelstoer closed this Jan 10, 2018

devsaurus mentioned this pull request Jan 18, 2018

added support for ads1015 #2231

Merged

4 tasks

devsaurus mentioned this pull request Jan 30, 2018

RFD - Structuring the sensor modules #2243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QMC5883L support for hmc5883l.c module (#2187) #2188

QMC5883L support for hmc5883l.c module (#2187) #2188

infrat commented Nov 29, 2017 •

edited

pjsg Nov 29, 2017

pjsg Nov 29, 2017

infrat Nov 29, 2017

infrat commented Nov 30, 2017

devsaurus commented Dec 17, 2017

infrat commented Dec 17, 2017 •

edited

pjsg commented Dec 18, 2017

edtamarin commented Dec 22, 2017 •

edited

marcelstoer commented Jan 8, 2018

infrat commented Jan 8, 2018

marcelstoer left a comment

devsaurus commented Jan 9, 2018

infrat commented Jan 9, 2018

devsaurus commented Jan 9, 2018

infrat commented Jan 10, 2018

marcelstoer commented Jan 10, 2018

devsaurus commented Jan 10, 2018

QMC5883L support for hmc5883l.c module (#2187) #2188

QMC5883L support for hmc5883l.c module (#2187) #2188

Conversation

infrat commented Nov 29, 2017 • edited

pjsg Nov 29, 2017

Choose a reason for hiding this comment

pjsg Nov 29, 2017

Choose a reason for hiding this comment

infrat Nov 29, 2017

Choose a reason for hiding this comment

infrat commented Nov 30, 2017

devsaurus commented Dec 17, 2017

infrat commented Dec 17, 2017 • edited

pjsg commented Dec 18, 2017

edtamarin commented Dec 22, 2017 • edited

marcelstoer commented Jan 8, 2018

infrat commented Jan 8, 2018

marcelstoer left a comment

Choose a reason for hiding this comment

devsaurus commented Jan 9, 2018

Pros

Cons

infrat commented Jan 9, 2018

devsaurus commented Jan 9, 2018

infrat commented Jan 10, 2018

marcelstoer commented Jan 10, 2018

devsaurus commented Jan 10, 2018

infrat commented Nov 29, 2017 •

edited

infrat commented Dec 17, 2017 •

edited

edtamarin commented Dec 22, 2017 •

edited