Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed i2c initialization #1

Merged
merged 1 commit into from
Sep 20, 2024
Merged

fixed i2c initialization #1

merged 1 commit into from
Sep 20, 2024

Conversation

virvel
Copy link

@virvel virvel commented Feb 27, 2024

Added missing initialization of I2CHandle. Should be working now. :)

@@ -51,6 +51,7 @@ inline int I2c::initI2C_RW(int bus, int address, int dummy)
}
cfg.address = i2cAddress; // this seems unused anyhow
i2cAddress = address;
i2cHandle.Init(cfg);
Copy link

Choose a reason for hiding this comment

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

yes! this is the main bit missing to get Trill to work!

Copy link

Choose a reason for hiding this comment

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

@giuliomoro add this fix to open a PR for review to E-S?

@giuliomoro
Copy link
Owner

thanks and apologies for missing the earlier notifications. @virvel your fix of course looks great (silly me). I wonder what happens when you instantiate more than one device. Is it going to behave OK?

@giuliomoro giuliomoro merged commit 112350d into giuliomoro:master Sep 20, 2024
@virvel
Copy link
Author

virvel commented Nov 18, 2024

thanks and apologies for missing the earlier notifications. @virvel your fix of course looks great (silly me). I wonder what happens when you instantiate more than one device. Is it going to behave OK?

great! no worries :) not sure about additional devices. could you make a pull request of this to electro-smith/libDaisy? maybe that was not your intention in the first place, but it would be nice to have it there. :)

@giuliomoro
Copy link
Owner

@virvel Sure, I'd be happy to make a PR there if you can confirm this works. I haven't had a chance to test it myself.

@virvel
Copy link
Author

virvel commented Nov 22, 2024

@giuliomoro It works using a single I2C device connected at least. I've been using the trill craft a lot with the daisy seed. :) I think @dromer has been using it successfully as well.
There has been a few minor changes to some parts of libDaisy since you made this, but it's easily fixed.

@dromer
Copy link

dromer commented Nov 23, 2024

@virvel not actively, mostly just did a couple tests since I have some Trill sensors that I'd like to use with libDaisy (hoping in combo with Heavy in the future).

This initialization fix at least allowed to using it as a PoC, but I think it would still need some work to properly integrate with libDaisy.

@giuliomoro
Copy link
Owner

What more work do you think is needed? The library itself is fully featured, but I understand that the example could have a bit more meat in it. Is there some libDaisy convention that I am missing ?

@dromer
Copy link

dromer commented Nov 23, 2024

Shouldn't the device config and init not follow the libDaisy conventions for this class of peripheral device?

https://github.com/electro-smith/libDaisy/blob/master/src/dev/tlv493d.h#L35-L71
https://github.com/electro-smith/libDaisy/blob/master/src/dev/mpr121.h#L23-L60

@giuliomoro
Copy link
Owner

@giuliomoro
Copy link
Owner

I rationalised the commits, rebased and it's now a PR electro-smith#657

@dromer
Copy link

dromer commented Nov 23, 2024

I guess what I mean is that the interface with libDaisy should follow the common OOP approach, where the rest of the Trill implementation is more functional.

Does that make sense?

@giuliomoro
Copy link
Owner

turns out things have changed upstream so the rebase fails CI ... let's have further discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants