Skip to content

Conversation

@bugadani
Copy link
Contributor

@bugadani bugadani commented Jul 25, 2023

Counterpart of #679. Submitted as a different PR because I can not test these and I would prefer not delaying the S3 pull request if this one is incorrect somehow.

If some of the code doesn't seem to make sense, you are right. But I just plucked the constants out of esp-idf, without simplifying them. This is particularly visible in S2, where the code:

  • disables a non-existing bluetooth (and I think the value mask overlaps with the other values)
  • and sets and clears the same bits at the same time (though, esp-idf does this in two register operations, but I don't think it matters).

I'm not touching C6 and H2, those are scary (i.e. they are different enough that I can't tell at a glance whether they are wrong)

@bugadani bugadani marked this pull request as ready for review July 25, 2023 07:54
@bugadani bugadani marked this pull request as draft July 27, 2023 18:20
@bugadani bugadani marked this pull request as ready for review July 28, 2023 09:31
@bugadani bugadani changed the title Fix ESP32 and ESP32-S2 radio clocks Fix ESP32/S2/C2/C3 radio clocks Jul 28, 2023
@bugadani bugadani force-pushed the radio_clocks_s2 branch 3 times, most recently from 77a72d2 to 19953a0 Compare August 1, 2023 17:18
@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 7, 2023

Works fine on S2,C2,C3.

On ESP32 WIFI also works fine but the BLE example crashes immediately:

INFO - COEX Version 2.0.0
ASSERT_PARAM(134220032 0), in lld.c at line 289


Exception occured 'IllegalInstruction'
Context
PC=0x40082edb       PS=0x00060b15
0x40082edb - r_assert
    at ??:??
0x00060b15 - PS_WOE
    at ??:??
A0=0x800832c1       A1=0x3ffc2320       A2=0x00000000       A3=0x08000900       A4=0x00000000
.........

@bugadani
Copy link
Contributor Author

bugadani commented Aug 7, 2023

I don't have a usable ESP32 at hand, unfortunately. Can you please tell if that crash is a result of this PR or one of the previous ones? Since this one mostly just modifies register values, I'm betting on the latter.

@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 7, 2023

I don't have a usable ESP32 at hand, unfortunately. Can you please tell if that crash is a result of this PR or one of the previous ones? Since this one mostly just modifies register values, I'm betting on the latter.

2f091161b40ee985a1f12ee43c412c045ae809af (the commit before 4c34c8d) still works

@bugadani
Copy link
Contributor Author

bugadani commented Aug 7, 2023

Bizarre. Thanks, I'll investigate somehow.

@bugadani
Copy link
Contributor Author

bugadani commented Aug 7, 2023

In the mean time, should I revert ESP32 and offer the rest up for consideration, or should we wait until some miracle happens? :)

@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 7, 2023

In the mean time, should I revert ESP32 and offer the rest up for consideration, or should we wait until some miracle happens? :)

Probably better to revert ESP32 and get the other code merged👍

@bugadani bugadani changed the title Fix ESP32/S2/C2/C3 radio clocks Fix ESP32-{S2/C2/C3} radio clocks Aug 7, 2023
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@bjoernQ bjoernQ merged commit da497c8 into esp-rs:main Aug 7, 2023
@bugadani bugadani deleted the radio_clocks_s2 branch August 7, 2023 15:20
playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Sep 26, 2023
* Fix ESP32S2 radio clocks

* Fix C2

* Fix C3

* Fix changelog
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.

2 participants