Added generic RPI cpu detection based on the revision string.

[penfold42]

So, who's sick of updating hw revisions?

For devices with the "new style" of revision string, this code will automatically determine the cpu type.

The new code is only used if we dont match on any of the list of devices (which I've left in there)

[penfold42]

Ping @jgarff

[jgarff]

Thanks @penfold42. Sorry for the late reply @penfold42. I'm a bit concerned with this changeset in that unknown boards, non-raspberry pi boards, or new versions of the Broadcom chipset will default to something that's not appropriate and crash the device (root + mmap). This is why the current mechanism is so specific, and why I haven't accepted changesets such as this in the past.

[Gadgetoid]

While I was very much in favour of this change, I can understand the rationale for not including it.

With the new error reporting in place it should be easier to recognise and add new boards as and when they are encountered. Although I appreciate this can cause some extra support load for people building rpi_ws281x into projects.

However- to play devil's advocate we've already seen that this library can, and will, cause unexpected things to happen even with this explicit list of supported boards. I could use this point to argue both for and against generic auto-detection.

For example, the recent issue with DMA and SD card corruption/read-only errors begs the argument; "since it can break anyway, why not remove any illusion of safety and document that this library does some low-level funky stuff?"

Anyway. I'm happy to shelve this PR and close it for now. Although perhaps it could be reintegrated in such a way that auto-detection is a - suitably documented - initialisation option for those who wish to use it?

[ajmas]

Should this PR be closed, based on above comments?

[Gadgetoid]

It's left open as a reminder to anyone who might have the same idea.

I'm conflicted now- I think we should update and merge this but include an allowed-list of hardware revision IDs which have been tested. Making the library thus:

1. Auto-detects the memory offsets using this approach
2. Includes a list of "tested" boards that we know don't melt or explode when used with this library
3. (Possibly) Includes a "force" option, that allows the user to run/test the library with an untested board

It's been over 3 years since this PR and we haven't had a major issue with a new board release - DMA channel/SD card conflicts weren't related and happened on tested boards - so perhaps it's time to embrace the best of both worlds. The rpihw.c file is getting pretty silly.

[timonsku]

I'm very strongly in favour of merging this PR.
With the "new" way of handling revision codes this is a cat and mouse game that has to happen every couple months at this point and is already causing unnecessary grief.
I understand the desire to be able to know if you are dealing with a known working or a new revision but I do not see the benefit in doing so at the cost of constantly breaking dependants who don't have direct control over this upstream repo who might need to update things sooner than is desired here or the maintainers here can handle.
This will otherwise lead to a lot of forks in the future just for the sake of being able to keep up to date, I'm close to forking both this and the python binding myself for this reason and I'd really rather not do that.

[Gadgetoid]

Right there with you @timonsku -

1. This hasn't prevented the issues we've had with certain hardware/software combinations from happening anyway
2. AFAIK there have been no issues relating to a specific hardware revision other than them not being supported...
3. Regardless of whether I fix & merge these upstream myself (I do many, maybe all of them these days?) I have to merge downstream and release a new Python library with nothing more than +1 or +2 extra compatible boards... that's work I'd love not to do!

And, perhaps the most damning:

4. I don't have 90% of the board revisions supported by this software, much less the time to actually test it with them and give any guarantee that they're properly and safely supported. As such this list of supported boards doesn't really make any promises or guarantees that rpi_ws281x will work now or in future.

I think @jgarff's original rationale made a lot of sense, but it's since been invalidated by the above and now there are simply too many boards/revisions to restore any faith that The Compatibility List is in any way useful or accurate.

I'm happy to forge ahead, but since this is @jgarff's baby I'd like some consensus from at least them and probably a yay from @penfold42 too. (plus maybe a rebase and a cursory check this still works? 😆)

[jgarff]

Thanks Philip. I think this is fine. I think my primary concern was that people would try to build and run this on non rapsberry pi boards, and we'd be flooded with support tickets for unsupported hardware. Are we likely to get false positives on non raspberry pi boards with this change?

…

On Mon, Mar 22, 2021 at 9:29 AM Philip Howard ***@***.***> wrote: Right there with you @timonsku <https://github.com/timonsku> - 1. This hasn't prevented the issues we've had with certain hardware/software combinations from happening anyway 2. AFAIK there have been no issues relating to a specific hardware revision other than them not being supported... 3. Regardless of whether I fix & merge these upstream myself (I do many, maybe all of them these days?) I have to merge downstream and release a new Python library with nothing more than +1 or +2 extra compatible boards... that's work I'd love not to do! And, perhaps the most damning: 1. I don't have 90% of the board revisions supported by this software, much less the time to actually *test* it with them and give any guarantee that they're properly and safely supported. As such this list of supported boards doesn't really make any promises or guarantees that rpi_ws281x will work now or in future. I think @jgarff <https://github.com/jgarff>'s original rationale made a lot of sense, but it's since been invalidated by the above and now there are simply too many boards/revisions to restore any faith that The Compatibility List is in any way useful or accurate. I'm happy to forge ahead, but since this is @jgarff <https://github.com/jgarff>'s baby I'd like some consensus from at least them and probably a yay from @penfold42 <https://github.com/penfold42> too. (plus maybe a rebase and a cursory check this still works? 😆) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#183 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACB55G6WOPFQ45YV4N5LNCTTE5O4VANCNFSM4DMGER7A> .

[timonsku]

It's definitely possible to 100% identify a Raspberry Pi CPU now a days. This was an issue with early Raspberry Pis and their old revision number scheme. The new one fixed that and has very little practical chance to collide with other SoCs. Should it ever be a concern, also parsing the Vendor string can make sure this would absolutely never happen.

[Gadgetoid]

Some kind of "This will absolutely only ever work on Raspberry Pi: link/to/github/issue/explaining/why" exception wouldn't be a bad idea. @timonsku have you got any more concrete ideas about how that could be implemented? I'm outrageously busy at the moment. (who isn't, I suppose 😬)

[timonsku]

I'm outrageously busy at the moment. (who isn't, I suppose 😬)

Same :)
The best way is probably to look at /proc/cpuinfo
The Hardware field will tell us if we are dealing with Pi SoC, it is always saying BCM2385 no matter what Pi.
If anything needs to happen based on the model then parsing the revision code is the way to go (the new style, not the old)
https://www.raspberrypi.org/documentation/hardware/raspberrypi/revision-codes/README.md

[Gadgetoid]

This is what we need for the correct memory offsets... sheesh this was almost 6 years ago - https://github.com/jgarff/rpi_ws281x/pull/104/files

[timonsku]

ah nice, anything standing in the way to merge this now a days?

[jgarff]

There was an unanswered change that I had requested on the merge if I remember correctly. Additionally I'm not sure the stuff we've talked about with regard to running on non-pi hardware was addressed in that ticket was it?

…

On Mon, Apr 12, 2021 at 2:57 PM Timon ***@***.***> wrote: ah nice, anything standing in the way to merge this now a days? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#183 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACB55GYMD3LZQDJ3ZYHV6MDTINNEJANCNFSM4DMGER7A> .

[ganzgustav22]

Can you guys please merge this after almost five years now?

[Gadgetoid]

@ganzgustav22 as alluded to in the following:

Additionally I'm not sure the stuff we've talked about with regard to running on non-pi hardware was addressed in that ticket was it?

Any change that opens the flood gates to this software running on any Pi revision, would need to additionally check that it is - in fact - running on a Raspberry Pi.

The potential repercussions for bashing memory mapped registers on an unsupported, third party board range anywhere from filesystem corruption (which has even happened on Pi's) to hardware damage.

As much as I would love to never have to release another "new Pi version supported" version for rpi_ws281x-python I would rather not - no matter how much the license might disclaim responsibility - be responsible for frying someone's Raspberry Pi knock-off.

Furthermore, I still think grabbing the ranges from device-tree is the more correct answer: #104 since parsing a hex string from /proc/cpuinfo is... kinda janky.

There's some documentation of device tree ranges here: https://forums.raspberrypi.com/viewtopic.php?t=320295

I'm not totally confident my method is complete:

static unsigned get_dt_ranges(const char *filename, unsigned offset)
{
    unsigned address = ~0;
    FILE *fp = fopen(filename, "rb");
    if (fp)
    {
        unsigned char buf[4];
        fseek(fp, offset, SEEK_SET);
        if (fread(buf, 1, sizeof buf, fp) == sizeof buf)
        address = buf[0] << 24 | buf[1] << 16 | buf[2] << 8 | buf[3] << 0;
        fclose(fp);
    }
    return address;
}

This - deprecated in favour of libgpiod - implementation seems more robust:

https://github.com/RPi-Distro/raspi-gpio/blob/c68e89eb3758715dabdc2c135ca99a905fb6b03b/raspi-gpio.c#L168-L211

So I think if we can:

1. Move to device-tree ranges
2. Come up with a satisfactory way to ensure the software is running on a Raspberry Pi

Then we might finally be able to put this to rest.

[timonsku]

What makes you feel parsing the revision code would be yanky?
It's what RPI gives you at hand for exactly this purpose to determine very clearly what you are dealing with and is something that will always be unique to a Pi. Something that will also not change by changes to the device tree in future OS releases.
https://www.raspberrypi.com/documentation/computers/raspberry-pi.html#new-style-revision-codes

There is also working code for this already with a compatible license: https://github.com/AndrewFromMelbourne/raspberry_pi_revision
It would just be a matter of making a decision and integrating it.

E: or are you interested in DT to get the peripheral base address? I'm not sure how stable that is in terms of future compatibility? Feels more dangerous than using the thing they promise to keep consistent.

[Gadgetoid]

I've seen no evidence to suggest the device tree method is likely to break in the near future so I don't have any concerns about relying upon it. Any evidence to the contrary would be welcomed, however.

The fundamental problem with relying on /proc/cpuinfo is that it's an unnecessary extra step. We want the ranges, so hard-coding them and checking for the CPU model is a very roundabout way to get there when endpoints exist to expose them directly.

This also means we trade maintaining individual Pi revisions, for having to add the necessary ranges every time a new SOC is released.

Of course the latter is... uncommon... and in fact having to explicitly add support on a SOC-by-SOC basis could even be argued as a feature. So... I might have argued myself over to your side here 😆

The hypothetical doesn't-explode-your-hardware checks the revision specific approach was supposed to enforce never really happened; but at least anyone running on a future, untested SOC would have to make a code change and assume responsibility for their Pi exploding.

The second argument against just /proc/cpuinfo Revision checks follows that although Raspberry Pi might guarantee it never breaks/changes, that doesn't grantee some third party knock-off board doesn't use a similar enough approach to pass muster as a Pi and explode. Using device tree doesn't fix this, of course... so either way we might need a robust way to check the code is running on a Pi. Maybe I'm overly cautious here, but if we're moving from matching whole Revision IDs to just a portion then we might as well go the full nine yards and get the info we want in the first place.

[timonsku]

I think the scenario of knock off revision codes is fairly unlikely :)
Personally I think its generally unlikely to truly cause issues. Having some check is reasonable but I would not go head over toes to ensure every edge case.
I'm open to any solution!
If you feel device tree has more benefits then lets go with that.

[timonsku]

I would have some time to work on this.
@Gadgetoid so just to get you right, you would like to do two things. Use the function you linked from raspi-gpio to fetch the right address and thus make the library forward compatible with unknown future hardware and then have another check that confirms that what we are getting back is actually from a BCM platform?

[Gadgetoid]

Yes and yes.

Though I think @jgarff should probably weigh in on whether or not we should abandon the pretense of testing on new SOC versions. I do believe it is merely a pretense and we probably shouldn't let it hold us back.

(also, thank you!)

[Gadgetoid]

Note that #350 might be worth taking into consideration, and actually lead me to glance over the aarch64 hacks which workaround the lack of (or inability to rely upon) /proc/cpuinfo. I guess that probably supports my assertion that we should use device-tree, but we might need to do some aarch64 testing (joooy) to make sure whatever is-this-a-raspberry-pi check we use actually works across the common distros.

Either way it would be nice to consolidate the aarch64 hack into whatever method we come up with here.

[Gadgetoid]

@jgarff possible additional case to be made here. Could rpi_ws281x be moved to the rpi-ws281x org?

I've been hesitant to ask, since I know a good, well-trafficked project can be a feather in your proverbial GitHub cap and I'm totally fine with the answer being "heck no!".

But - in addition to it being a sensible choice for the ongoing health of the project, it might be reasonable to suggest it could absolve you of any negative consequences from choices like the above. (And #453 for example)