Skip to content

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Jan 17, 2019

Contribution description

This PR provides a default I2C configuration for the nrf52 Thingy52 and add hts221 sensor as dependency when saul is used. lps22hb could also be added when #10695 will be merged. ccs811 requires support for the GPIO expander SX1509.

This PR is untested, I don't have this hardware. Maybe @haukepetersen will be able to test this.

Testing procedure

  • Build and flash tests/driver_hts221 and verify the output is correct:
    $ make BOARD=thingy52 -C tests/driver_hts221 flash term
  • Build and flash examples/saul and check the hts221 and lps22hb values:
    $ make BOARD=thingy52 -C examples/saul flash term

Issues/PRs references

None

@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Jan 17, 2019
@aabadie aabadie requested a review from haukepetersen January 17, 2019 09:14
@maribu maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Jan 30, 2019
@maribu
Copy link
Member

maribu commented Jan 30, 2019

I will test this tomorrow

@aabadie
Copy link
Contributor Author

aabadie commented Feb 26, 2019

Maybe @haukepetersen could test this PR on its Thingy52 ?

@aabadie aabadie force-pushed the pr/boards/thingy52_i2c branch from 4c5c9ab to 5f263d4 Compare February 27, 2019 20:51
@PeterKietzmann
Copy link
Member

I will test this tomorrow

@maribu did you?

@maribu
Copy link
Member

maribu commented Mar 5, 2019

Sadly not. I wanted to borrow my colleague's Thingy:52 for testing another PR and this one, but the Thingy:52 went missing. I only put this information in the other PR and forgot about this one.

@Citrullin
Copy link
Contributor

Citrullin commented Apr 6, 2019

We have a lot of these Thingy52. If somebody needs one, I probably can give a few to you. I just have to ask my boss first. (I don't think that will be a big problem) So, ping me, if you need one :)

@Citrullin
Copy link
Contributor

The hts221 should automaticlly be found when I use saul_default, right?
So when I use saul in the shell the sensor should be there, right?

@aabadie
Copy link
Contributor Author

aabadie commented Apr 7, 2019

Thanks for having a look @Citrullin !

So when I use saul in the shell the sensor should be there, right?

Yes, that's the idea. The tests/driver_hts221 application should also work.

@MrKevinWeiss MrKevinWeiss self-requested a review May 6, 2019 10:50
@MrKevinWeiss
Copy link
Contributor

I cannot get an ack from the hts221, I tried 0x5F address and it doesn't work... The 0x19 address works though which is weird since it should be connected to I2C_DEV 1 not 0...
I believe it has something to do with the power settings. It appears the VDD is actually controlled by this VDD_PWD_CTRL pin (p0.30). It seems a high turns the power on and low turns the power off.

@aabadie
Copy link
Contributor Author

aabadie commented May 10, 2019

Thanks for testing @MrKevinWeiss! I'll have another look when time permits and will try to update.

@MrKevinWeiss
Copy link
Contributor

I can also return to it (when time permits as well), I tried a bit but couldn't get it going.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019

Now that #12290 was merged (with the same I2C configuration provided by this PR...), I rebased and also added support for lps22hb (that was merged in the meantime as well).

I also found why @MrKevinWeiss couldn't make it work. Will open a PR now. The bug is pretty awesome.

@aabadie aabadie changed the title boards/thingy52: configure i2c and add on-board hts221 dependency boards/thingy52: configure i2c and add on-board hts221/lps22hb dependency Oct 4, 2019
@MrKevinWeiss MrKevinWeiss added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 4, 2019
@aabadie aabadie changed the title boards/thingy52: configure i2c and add on-board hts221/lps22hb dependency boards/thingy52: add dependency for on-board hts221/lps22hb sensors Oct 4, 2019
@MrKevinWeiss
Copy link
Contributor

MrKevinWeiss commented Oct 4, 2019

Ya it still seems like the I2C itself is having some problems. I cannot even use the i2c_scan command...
EDIT
it seems like I can use the scan command on device 1 but not 0 as it locks up.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019

There might be other problems then. I already fixed another one with lps22hb: the address was not correct.

@MrKevinWeiss
Copy link
Contributor

Hmm ya. With the two PRs I get lockup on i2c dev 0 and can read and scan on address 0x19 on i2c dev 1. With master both 1 and 0 behave like one (which is incorrect). I don't know why the terminal locks up on i2c dev 0 though...

@MrKevinWeiss
Copy link
Contributor

I would use make BOARD=thingy52 -C tests/periph_i2c/ flash term and USEMODULE=i2c_scan make BOARD=thingy52 -C tests/shell/ flash term to show what I am talking about.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019

It appears the VDD is actually controlled by this VDD_PWD_CTRL pin (p0.30)

Can you try to initialize and set this pin high in board init ?

@aabadie aabadie force-pushed the pr/boards/thingy52_i2c branch from e70f2ac to 0b0468d Compare October 4, 2019 12:50
@MrKevinWeiss
Copy link
Contributor

Yup it seems to have fix it.

So adding

void board_init(void)
{
    gpio_init(GPIO_PIN(0, 30), GPIO_OUT);
    gpio_set(GPIO_PIN(0, 30));
    /* initialize the CPU */
    cpu_init();
}

to both the #12372 and this PR then using the i2c_scan command on i2c_dev 0, no longer freezes the terminal but gives the following output.

> i2c_scan 0
2019-10-04 15:01:41,023 #  i2c_scan 0
2019-10-04 15:01:41,023 # Scanning I2C device 0...
2019-10-04 15:01:41,024 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2019-10-04 15:01:41,024 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2019-10-04 15:01:41,024 # 0x00 R R R R R R R R R R R R R R - -
2019-10-04 15:01:41,035 # 0x10 - - - - - - - - - - - - - - - -
2019-10-04 15:01:41,035 # 0x20 - - - - - - - - - - - - - - - -
2019-10-04 15:01:41,035 # 0x30 - - - - - - - - X - - - - - X -
2019-10-04 15:01:41,035 # 0x40 - - - - - - - - - - - - - - - -
2019-10-04 15:01:41,036 # 0x50 - - - - - - - - - - - - X - - X
2019-10-04 15:01:41,036 # 0x60 - - - - - - - - - - - - - - - -
2019-10-04 15:01:41,036 # 0x70 - - - - - - - - R R R R R R R R

I still don't understand why the terminal is frozen. Maybe it could be that the i2c pins are actually held down and it is waiting on a flag or something. If the sda and scl pins were high impedance I would expect them to just return an error code.

@aabadie aabadie force-pushed the pr/boards/thingy52_i2c branch from 0b0468d to 7b33599 Compare October 4, 2019 14:00
@aabadie aabadie removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 4, 2019
@aabadie
Copy link
Contributor Author

aabadie commented Oct 4, 2019

@MrKevinWeiss, I rebased on latest master and added the management of VDD pin in board_init (note that this might have other low-power problems, according to the user manual). Can you give this another look ?

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 4, 2019
@aabadie aabadie force-pushed the pr/boards/thingy52_i2c branch from 3981f4c to c5664b7 Compare October 4, 2019 17:54
@MrKevinWeiss
Copy link
Contributor

I am out of office until Monday. Maybe ping @leandrolanzieri or @jia200x.

@MrKevinWeiss
Copy link
Contributor

Well it seems to work now however the temperatures and humidity seem off...

2019-10-14 10:11:05,685 # Init HTS221 on I2C_DEV(0)
2019-10-14 10:11:05,685 # H: 0.0%, T: 8.9°C
2019-10-14 10:11:05,685 # H: 0.0%, T: 8.2°C
2019-10-14 10:11:07,657 # H: 0.0%, T: 8.2°C

also on the saul example

> saul read 1
2019-10-14 10:14:33,392 #  saul read 1
2019-10-14 10:14:33,392 # Reading from #1 (hts221|SENSE_TEMP)
2019-10-14 10:14:33,392 # Data:	     9.3°C

I don't know if getting the correct values is out of scope of this PR or not... It isn't that cold in the office.

@aabadie
Copy link
Contributor Author

aabadie commented Oct 14, 2019

Does the lps22hb driver work ?

@MrKevinWeiss
Copy link
Contributor

Yup, that seems OK

> saul read 4
2019-10-14 10:48:14,685 #  saul read 4
2019-10-14 10:48:14,686 # Reading from #4 (lps22hb|SENSE_PRESS)
2019-10-14 10:48:14,686 # Data:	       1013 hPa
> saul read 5
2019-10-14 10:48:18,090 #  saul read 5
2019-10-14 10:48:18,091 # Reading from #5 (lps22hb|SENSE_TEMP)
2019-10-14 10:48:18,091 # Data:	     24.98°C

@MrKevinWeiss
Copy link
Contributor

If you want this in I would just raise an issue for the HTS221. I or @JannesVolkens can look into it later.

@MrKevinWeiss
Copy link
Contributor

Looking at the datasheet it may also be improperly calibrated... I would need further investigation but the more I look into this the more I think this is out of scope with the PR. Any objections @aabadie ?

@aabadie
Copy link
Contributor Author

aabadie commented Oct 14, 2019

Any objections @aabadie ?

Please ACK :)

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

I will raise the issue for the sensor.

@MrKevinWeiss MrKevinWeiss added Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 14, 2019
@MrKevinWeiss MrKevinWeiss merged commit e236fa1 into RIOT-OS:master Oct 14, 2019
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
@aabadie aabadie deleted the pr/boards/thingy52_i2c branch March 5, 2020 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants