20201014

My First Solar Install - Notes and Experiences (Part 2)

During our serious investigations and talks with potential contractors, we were introduced to PVWatts (as mentioned in my last post).  With the rough estimate, I worked out probably yearly and monthly production numbers based on the PVWatts simulation.  Now, I have a more complete and near-final layout in hand.  Time to plug in the numbers!

The Layout

Certain building codes require set-backs.  I didn't know about those when I was first playing with the idea.  You evidently also cannot saw the vent pipes shorter (not that I really expected that to be allowable).


We had to sign off on the splits in the arrays, otherwise all the panels (at least on the east face) would be together.  The split was caused by a vent pipe in an inconvenient spot.  I don't care much about aesthetics, personally - I'd rather have MORE PANELS!

Simulation Configuration

The orientations of each roof is given in red, in degrees.  The 4/12 pitch comes to about 18.4 degrees.  Surprisingly, an extra panel was able to be added to the south-facing roof.

The configuration I used for each roof in PVWatts was:
  • DC System Size: 320W * number of panels / 1000 (kW)
  • Module Type: Premium
  • Array Type: Fixed (roof mount)
  • System Losses: 14.08% (default)
  • Tilt: 18.4
  • Azimuth: azimuth of each roof

Numerical Results

I didn't bother with PVWatt's power cost estimates, since my utility is...a little complicated.  That said, here are the results:


This should provide about 2,600 kWh per month (average) production, 200 kWh more than my original target.  Good thing, since certain people in my household have recently taken to extra-long, extra-hot showers.

Column details:

  • AC System Output (kWh): The amount of power the solar system should generate for each month.
  • Expected kWh Usage: Based on my 2020 usage, with the last three months extrapolated.
  • Production Over/Under: Positive numbers indicate we produced more power than we consumed.
  • Credits: If we produced more than we consumed, the provider will roll our overage.
  • Energy after Solar: The amount of energy we didn't cover with solar (which is the negative of our Production Over/Under, not ironically).
  • Credits Accumulated: Since our provider rolls credits month-to-month, this is the "current balance" for the given month.  Cannot go below zero.
  • Energy after Credits: The amount of power not covered by both solar and credits.
  • Energy Costs: Cost of our energy usage after everything has been accounted for.
  • Costs without Solar: What it would cost us for the power used, without solar input.
Note that the costs are at my provider's current rate schedule, and are not extrapolated beyond the anticipated inputs.  Also, for simplicity this considers a constant rate schedule, whereas my provider tends to change their rates several times during the year.

Whereas most companies will estimate how many hundreds of thousands of dollars you'll save in the future, this table tries to estimate what we might have saved or spent for just the power we used in 2020, with the system as designed.  With energy rates expected to increase at around 4% per year for our provider (YMMV), the "savings" as it were should only go up.

This table does not show how much the system will cost on a monthly basis (we're anticipating around $250/month), so the final Savings number is slightly misleading.  But, if we saved $3,806 on actual energy costs for the year, and spent $3,000 on the loan, we'd be $806 ahead.  That's over three months of payments we could make to pay down the loan quicker (or about 100 sub sandwiches a year from our favorite sandwich shop - my preferred way to calculate money saved and spent).

Heat Losses and Discussion

I had to do a little digging, but PVWatts actually considers performance degradation due to heat.  This is encoded in the Module Type parameter.  Their loss number was a little nicer than my actual panel specs:
  • PVWatts power loss due to heat: -0.35% per C
  • My panel's power loss due to heat: -0.37% per C
But, with such a small percent, we are not talking a considerable amount.  After all, at +30C operating temperature, we're talking a difference between 10.5% losses and 11.1% (0.6% off).  Just roughly, that amounts to 1 kWh less production for the year, if I did the math right.  Bear in mind that a lot of marketing for panels, inverters, etc, will tout efficiency improvements around 1-2%.  At the end of the day, what matters is how much power it produces, and whether or not that power is worth the cost of those percentage points.

So, let's call that "margin of error," especially since this tool is also using historical weather data from a location at least an hour away to drive its simulation.

If things go according to plan, we should see an insufficient production for the first two months, but then zero energy costs (aside from the monthly fee) through to the end of the year.  If we're able to run the house at a lower consumption rate, that will just mean a better buyback at the end of the year.  If panel performance isn't as good as we expect, we'll (at worse, hopefully) be paying upwards of $300 for energy for the year.  That amount is based on double the system losses due to heat (which I added in before realizing PVWatts already accounted for it).

Guess we'll see how it goes!




20201009

My First Solar Install - Notes and Experiences (Part 1)

We just signed up for a solar install.

O.M.G.

One horrifying power bill after another, and the looming lowering of the Solar Energy Tax Credit, and it was time to do something drastic.

Some Basics You Need To Know

Cost per Watt

It's common to compare systems in terms of cost per watt.  That's the total cost you pay for the materials and install, divided by the watts your system is rated for.  If you put 30 panels on your roof, and each panel produces 290 watts (max output), that's a 8,700 watt system (30 panels * 290 w/panel).

If that system costs you $28,275 out of pocket (i.e. before incentives, rebates, or tax credits), then your system cost you $28,275 / 8,700, or $3.25 per watt.  Using this, you can create a rough comparison of vendors and their offerings.  There are finer details to consider, but if you have two vendors offering roughly the same products, and you want the lower price (often, but not always, a good choice), you can reduce them to price per watts and go from there.

Propaganda - I mean, Sales Pitches...

Here are some things you'll hear and read:

  • Over 30 years, you'll pay X dollars in utility power, and it's only getting more expensive.
  • These panels offer the highest efficiency/wattage on the market.
  • These inverters/optimizers are 99% efficient.
  • Solar payments will be lower than your existing power bill.
  • Warranties!!!!
First, yes, power is getting more expensive every year.  They usually also tell you how much you'll save by going solar.  This is a gimme.  The numbers are legit, but let's be real, this is an argument to go solar, not to go with a particular company.

The second is not untrue, but higher efficiency and higher wattage panels usually come at higher cost.  They will take up less room on your roof for the same kWs, by the numbers.  That's great if you don't have a lot of space, or have high-enough needs that you need to squeeze every watt you can out of every square-foot of roof.  But will you see a return on that investment?  For example, a 21kW system will require 65 panels at 325W per panel, and 55 panels at 385W.  At $209 and $319 (prices of a couple of comparable panels I pulled from a solar component reseller), this yields $13,585 and $17,545, respectively, for the same total kW of generation.  You need to crunch the numbers and not get blown away a mere data-point.  

The third is another data-point, and this one makes me mad because it's misleading when presented the way it is.  Optimizers and inverters condition or change the power (respectively), and to do this, they need electronics.  No electronics are 100% efficient.  So, touting this is just a comparison between one brand of optimizer with another, or one brand of microinverter with another.  For our calculations, the efficiency they report translates to how much power you're going to lose to your optimizer/inverter.  They consume power to operate.

Fourth, generally another gimme, but be careful.  Companies often show up and look at your bill, then quote you a system that is $20-$50 less per month.  But the price per watt might be through the roof (read on for examples, and no pun intended)!  What seems like a great deal, relative to your current energy costs, might not be a great deal compared with other offerings.  Even the same tech installed by a different company can yield savings.

Finally...Warranties.  OK.  These appear to be important.  But like the other points, they need to be considered in the overall picture.  Most solar tech seems to be quite robust, so offering a 25 year warranty on something that has an extremely low probability of failing is, well, almost pointless.  Not entirely, but...almost.  But everyone is doing it, and if it signs up more customers, so be it.  On the other hand, at least you have that level of assurance that your new system will reach its maturity (and then some) before it starts having costly problems.  If you opt for a more complicated system (i.e. microinverter or optimizer), those warranties will probably come in very handy.

Our Usage Needs

A 12-month moving average of our kW usage showed a recent usage trend of around 2300 kW / month.  I set our target for 2400 kW / month of generation.

We have about 5.2 sun-hours (yearly low) in my location, and luckily no trees or buildings to shade our roof.  We could expect to get a decent amount of input.  If our per-day goal is 80 kW, then we'd need a system at least 15 kW in size.  That's the layman's sizing; actual sizing is more complicated, and needs to take into consideration the fact that you won't generate that much power from your array every day, or every month (seasons, sun location in the sky, etc).

Every system we were quoted went above 17kW. Where I'm at, any system about 11.2kW is called a "type 2" system, and requires more gravy to get the train moving.

DIY Solar Options

If you are willing to forego better warranties, and you can either find someone to install it, or are willing to drill a few hundred holes in your own roof and reap that harvest during the next big rain, then you can buy kit systems online for $1.50 / W.  I'd certainly seriously consider it for a ground-based system, but not a roof system.  That said, the law may also not be on your side (and it wasn't on mine).

We need a certified installer to make the power company happy, otherwise no grid-tie for us.  Why grid-tie?  Batteries are too expensive, the ROI is just not there; and grid-tie is really the only option here.  So, we had to find a vendor.  We asked around for buy-and-install pricing, and found exactly one willing to do it, but the warranties offered on whole systems were just too appealing.  Plus, there is a ton of work and permitting that has to be done with a solar install.  

If you can do it, go do it.  If you are old and tired, and have multiple kids driving you crazy and a need to get stuff done, better to let a good installer take it on.

Finding a Vendor

After a fateful visit from a door-to-door solar salesman, I started deep-digging into vendors.

I found a list of the best local installers, who had hundreds of ratings and reviews each, plus certifications or accreditations from NABCEP (that's important): The North American Board of Certified Energy Practitioners.  I contacted four of them.  At least one was based in California.  The others were local of various size.

Spoiler Alert: a local company, and NOT the door-to-door guy, came out on top.  A good company also shouldn't hound you or pressure you.  Watch out for gimmicks like "improving your A/C system to cut down on your solar bill" - these just equate to easy money for them, and significantly higher $/W for you from your solar system.  And no, the guy who offered me that didn't have a crystal ball, but he should have for making such sweeping predictions about how great my A/C would be after that $5,200 treatment.

My takeaway: If you have already optimized your energy savings in your house, then size your system to cover your needs.  Don't under-size a system in anticipation of how much power you can maybe save after fixing your home's energy leaks. That's just putting the cart before the horse.

The System Quotes

All sorts of different systems were offered:  string-inverter, optimizer, micro-inverter, different kWs, even one A/C treatment.   Names hidden, we received the following quotes (cost per watt):
  • A: $2.82 / W
  • B: $3.11 / W
  • C: $3.40 / W
  • D: $4.02 / W
Lower is better for the wallet, but only if the other features match your needs and expectations.  We were offered systems of sizes:
  • A: 22.4 kW (string-inverter)
  • B: 19.24 kW (optimizer)
  • C: 22.11 kW (micro-inverter)
  • D: 17.985 kW (micro-inverter)
All $/W values are raw (before tax credits).

The Technology

There's a lot of information, and misinformation, on the Internet.  I'll try to sum up the realities here.

Inverter Tech

You have three major choices.  All inverters need to find the Maximum Power Point (MPP) of the panels - this the point in the panel IV curve that gives maximum Power output (where Power is Current (I) times Voltage (V)).  It's the "knee" in the IV diagrams you'll see for panels, where it stops going sideways and starts plummeting like a rock.

The MPP varies with sun, shade, soiling, etc.  There are different algorithms for finding it and keeping it optimal.  A less-intelligent inverter can get stuck in a local maxima.  Smarter ones will occasionally sweep to ensure the global maxima is actually the one it's on.

The String Inverter

Large inverters on the side of your house or garage, ideally near the meter.  No electronics on your roof, just panels.  SMA makes one of the best, and claims to compete with the newer tech.  

Overall, it's a simple, durable system with the fewest parts, and time-proven technology.  On the downside, it might not offer the overall energy generation of newer tech.  One reason for this might be it doesn't optimize as frequently as it could or should; another might be that individual panel losses add up more in a system like this.  However, take all this with a grain of salt: the research on all these claims is extremely lacking.

Optimizers

Basically DC-to-DC power conditioners.  They send ideal voltage to a central inverter, modulating the amperage accordingly all over the array.  The idea is to make the inverter work as efficiently as possible, by insuring it always has the right voltage.  An under-performing panel will be adjusted at the optimizer, along with all the related higher-performing panels, such that the total voltage is what's expected.

There are costs for this: every panel needs an optimizer.  The optimizers need power to do their job, and they are not 100% efficient at the transformation.  The sales sheets will tout 98% efficiency like it's a great thing (and compared to less efficiency, it is).  But that's 2% loss from every panel during the conversion process.  What that means in terms of actual power production, I do not know.  It could be 2% voltage loss, or 2% amperage loss, or even 2% overall power loss.

If it's a power loss, a 70 panel system at 320 W per panel would lose 462W due to the conversion loss.

Also, because every panel needs one, they are up there on the hot, hot roof, under a hot, hot panel.  So, there is a reliability concern.  You also still have a central inverter, but it might be smaller than the string inverter unit.

On the upside, if they do indeed perform better in intermittent shade scenarios, the losses during high-sun might be made up in gains during less optimal solar conditions.  You also get per-panel statistics, which is nice for diagnosing panel issues...or, I guess, bad optimizers.

Micro Inverters

These are like optimizers, but instead of doing DC-to-DC, they do DC-to-AC right at the panel.  No central AC inverter is required.  I'm not sure if they require any special junction equipment, but if you wanted ultimate efficiency with maximum shade tolerance, this is probably the way to go.

Of course, the cost here is that the micro-inverters are significantly more complicated than optimizers, and like optimizers you need one for each panel, at the panel itself.  So, complicated electronics in a harsh environment.  Again.

Like optimizers, you get per-panel info.  You also get a pretty hefty price-tag increase.

But What the Heck is a "String"?!

If you have 70 panels, you're not going to run 70 individual wires down your wall into your inverter or meter box.  You're going to group, or "string," them together, upwards of six at a time.  Usually wired serially, to kick up the voltage while keeping the amps relatively low.  Each string will run down to the inverter.  Each inverter can take in so many strings.  

For the SMA, a 70 panel system will require 3 SMA boxes (at least of the model we're getting - OOH SPOILER ALERT!): 6 panels per string, 4 strings per inverter, 3 inverters.

Panel Tech

People like to say that panels wired serially in a string will fail like Christmas tree lights.  But that is not the case for most modern panels.  For both power production and - more importantly - fire concerns, bypass diodes are pretty standard.  Most panels seem to set up groups of cells, and wire them to a bypass diode, with multiple diodes in a panel.  This means that if part of the panel is obstructed or fails, the other groups of cells can keep working.  If the whole panel is obstructed or fails, the whole panel can be bypassed.

Since bypass diodes are a thing, it begs the question of how useful optimizers are in such scenarios.  A somewhat contentious study by a university in Denmark suggests that the power costs of optimizers outweigh their benefits, except where (as stated above) intermittent shading or whole-panel issues are a thing.  You can read it here: https://www.sdu.dk/-/media/files/om_sdu/centre/cie/optimizer+for+pv+modules+ver11_final.pdf

Panel efficiency is a rather stupid number: aside from telling you how many watts per square meter you can generate, it tells you really nothing else, and ceases to be applicable very quickly during most actual operating conditions.  If you really wanted to compare apples-to-apples, and you had nothing else handy, panel efficiency will tell you the panel that will give you the most for the least space.  Is that the whole truth, though?  No.

There are two kinds of degradation you have to be aware of, at least to filter out the bull that will be handed to you by the sales guys: temperature, and standard lifetime panel degradation.

Panels perform worse when hot, and they give their stats on their spec sheets.  I studied two: a 320W and a 370W panel.  The 320W will put out 320W in optimal light conditions (read the spec sheet if you want to know what they are, and don't expect them to be numbers that match actual sun-obtained reality), at 25C.  My roof can easily get to 70C in places during a nice hot day, which should drop the 320W panel's production to 280W.

Panels degrade over time as well.  Industry standard warranty is 80% production at the 20 year mark, with the degradation being linear (meaning you should NOT see a lousy 80% production on year 3).  Below I'll talk about how linear degradation isn't quite the selling point the salespeople wish it was.

An Estimate - PVWatts

The PVWatts tool is pretty awesome: you can enter in your address, punch in some configuration details, and get a simulation on how your system will perform.  I did this for the two top systems quoted to us.  In order to get relevant results, I had to break the systems down into three estimates for the tool, since the tool doesn't handle multiple roof faces. 

Here's my yearly production estimates, after adding my roof simulations together:
  • A: 22.4kW => 31,101 kWh
  • B: 19.2kW => 27,071 kWh
I can only hope that PVWatts actually accounted for panel losses due to heat, but I honestly am not sure how much it did.  It's probably in the tool documentation.  One thing I could do, however, is calculate system losses over time.  Given each systems' panel specs, I calculated the amount of degradation I could expect at the 5, 10, 15, and 20 year marks.

At the 20 year mark, I had the following two numbers:
  • A: 22.4kW => 26,125 kWh
  • B: 19.2kW => 25,643 kWh
Even though the A panels degraded faster, the larger (yet cheaper per watt) system still produced more at the 20 year mark.

Extrapolating for Like-Kind Analysis

Is it worth it to beef up the B system and get both the better life and greater overall generation?  Let's look.

If we resized the B system to match its sizing to the A system, it should produce 31,582 kWh per year on year 1.  For that amount of array, the price tag would increase by $9,952.  An already expensive system (by comparison to A) would go up in cost by 16%.  This, to get a return of 481 kWh over the course of the year.

Other Considerations

There are other things you need to look into, when shopping solar installers.  They are just as important as the tech.

Financing versus Cash Price

Some vendors will sell you a much better cash-price, though one potential downside of that is you might not be able to claim any closing costs or origination fees for the federal tax credit.  Of course, that'll all be moot once the tax credit vanishes.

Financing Terms

Most vendors will sell 20 year financing.  25 years is probably not a good deal, and the "lower" payment might just be equal to or worse than a better 20 year system.  The internal pricing of parts and labor is completely opaque until after you've signed (and even then you might not see it).  Shopping around seems to be the only way to ensure that both your terms, and your total system costs, are optimal.

Warranties

A good vendor offers warranties.  Our select vendor offered warranties from the manufacturer (so we're told) that go beyond the standard, due to their rating with the NABCEP.

Panel warranties come in two parts: performance, and the panel itself.  The performance warranty covers that linear degradation I discussed above.  The panel warranty should cover a panel outright failing, de-laminating, or suffering some other catastrophic pre-end-of-life fault.

The roof penetration warranty is your installer saying your roof won't leak for some period of time after the install is complete.  Ideally, it's at least the life of the roof.  One vendor put a cap on top of that (albeit it was 25 years).  Our selected vendor said they'd restart the warranty following a reroof reinstall, and warranty for the life of the new roof.

The inverter needs a warranty as well, and it will often be different from the panels.

Finally, a workmanship warranty will cover a job-well-done (or maybe not so much).  The longer the better: if something goes belly-up, the vendor shouldn't be charging you to fix their mistakes.

Reroof Costs

If you need a new roof, it is not advisable to let Handy Roofer Dude take your Expensive High Voltage Solar System off your roof.  Let the solar installer do it.  It's another cost, of course, but varies widely.  Prices are sometimes listed per panel (like $50/panel or $100/panel for take-down and reinstall, plus storage fees).  Prices are also sometimes given in flat rates.  Most vendors couldn't guarantee what the panel removal and reinstallation costs in the future would be, but I settled for a "right now" cost estimate.

The Tax Credit

Get it while you can.  Once you do, you have five (5) tax seasons in which to fully recover it.  This means that if you can't claim enough taxes to get your full credit from the IRS, you can roll over the remainder to the next year.  After five years, you're out of luck, but I imagine for most people this won't be a problem.

Secondly, my thinking is that it's stupid to spend a dollar to save a quarter.  While it may be appealing to buy a larger system to get a larger tax credit, think about what you're doing here:

First, every solar loan operated with the expectation that you'd take that nice, fat tax credit and stuff it into your loan repayment by the 18 month mark, otherwise they'd re-amortize you at a higher rate.  Ironically, at least with one lender if you pay more than that by the 18 month point, you can get re-amortized to a lower monthly payment.

Second, the overall cost of the system is something you have to pay, credit or no credit.  Even after the tax credit, you're only getting a 26% discount (for 2020).  A $60,000 system is cheaper than an $80,000 system, and it will STILL be cheaper after you take that 26% off the price tag.  It seems to me the best way to overpay for your solar is to try to get your credit as large as possible.  Just Don't Do It.  Get a good system and let your actual power savings do the rest.

Additional Insurance

We have to obtain a $1 million general liability policy.  That amounts to about $19/month with our insurance provider, and is called an "umbrella policy".  Not every power company requires this, but ours does.  Lovely.  It's something to keep in mind, though, when you think about your total monthly cost.

Unused kW Credits

If I produce more power than I consume, my power company will roll those "credits" from month to month.  The idea here is that I can generate more than I use in the cooler months, and use more than I generate in the hotter months, and still come out net-zero for overall power cost.  At the end of the year, my company will supposedly "buy back" the unused credits, effectively zeroing me out to start afresh on the new year.

The buy-back isn't much - it's significantly lower than what they charge for their power.  But meh.  As for the production goals, I took a close look at the per-month report from PVWatts.  After comparing each month of simulated generation to the previous matching billing cycles, I will probably pay for some power in January and February.  The rest of the year I should be good.  I should generate enough to get a bit of a refund back.  In real numbers, I should go from spending $3900 / year on power to $133.

Remember, however, I still have a loan to deal with.  That's the trade off.  But, if the utility rates continue to increase, it will speed up my effective ROI, since that's more expensive power that I'm no longer buying.  I trade a highly variable, increasing power bill for a relatively stable and fixed solar loan.  The catch to this is generating enough to make that a reality.

In Conclusion

There are many ways to analyze the value and return of solar.
  • In 10 years, I would have paid my power company enough to have bought my chosen system for cash (after tax credits).
  • In about 16 years, I should see my ROI from the amount of savings I should get by going solar.  (That's my break-even point)
  • After the system is paid off, it will continue to generate power, even if we move and rent out the house.  Offering a relatively fixed, low power bill to the renters will probably be very alluring.
That said, what remains to be seen is the actual system install and performance.  I will write again once I have more to say about that.  The install process should take only about 3 days, including a meter panel swap.  We're going with the string-inverter system, for simplicity and because our shading is practically non-existent.  It's hoped that however the panels are wired, they will perform optimally regardless and we should see at least what PVWatts estimated for production.

20200520

Logitech G920 Steering Wheel Force Feedback on Linux...VERSUS EuroTruck Simlutator 2 - A LESSON IN KERNEL MODS

My kids are asking for their "truck game," and my wife is ardent she doesn't want the steering wheel hooked back up to her computer (for sake of her desk-space).  Can't say I blame her.  However, her system has the only kernel mods that make the force feedback (FF) work.  These mods were written by Yours Truly (that is, ME).  Time to port them, and while doing so write up a sort essay on the glory.

Here we go.

I wrote this patch about two years ago...sorry.  I have yet to submit it to upstream.  I'm actually in the process of re-testing against a recent kernel so that I can do just that.  In the meantime, if you want your steering experience on ETS2 to not suck, you can try building your own kernel with the patch included below.  (Jump to the very end for the goodies.)

Just a bit of background on the problem...

The Linux force feedback architecture naively assumes two things:
  1. That a program will politely send force feedback commands in a gentle, trickling stream.
  2. That the force feedback device will consume USB commands - and thereby rip through the workqueue - at the speed of light.
ETS2 breaks both of these assumptions by spamming the force feedback system at absurdly high rates, while the kernel pops at whatever rate it's able to, quickly overwhelming the driver and queuing up a MASSIVE batch of commands that the driver will eventually churn through...by which time you'll have decided that ETS2 + Linux FF sucks, you turn off the game and momentarily think about reinstalling Windows (immediately after which you spontaneously vomit).

I got my first hint at this after going through the above experience, and then exiting the game and watching my wheel bump and thump and shudder and rumble for about 3 minutes.  I was pondering... Something seemed non-random about the things it was doing, and then it suddenly STOPPED.  I realized it had been playing all the effects I had just experienced, just really, really, really, really late.

In lieu of waiting for the ETS2 developers to fix their code, I decided to take a stab at the Logitech FF subsystem in the driver itself.  I'll spare you the gory details of my testing.  Ultimately, I built something that works well enough.  And by well enough, I mean that I absolutely LOVE the FF in ETS2 now, it works great.

But I'm a programmer, so I have to say "there is definitely room for improvement," but here's what it does:

The basic premise is that we have only certain things we do to a FF device.  We can create and delete effects.  We can play and stop them.  We can update them.  That's about it.

If we update something a bunch of times, it doesn't make sense to keep the old updates queued and waiting to be sent.  We assume the device just cares about the last update for a given effect, so we just save that and flag that there's an update pending for that specific effect number.  Future updates overwrite our queued update, so that only the latest update is sent by the time the workqueue gets around to this request.

If we play and then stop something, but the driver hasn't gotten around to playing it at all, then it's as though it had never been played to begin with.  Don't send the play event.  Likewise, if we stop and then play an effect again, it's like it never stopped (so to speak).  We might queue up a single play event to restart it, but ultimately it's a single event.  

If we queue a bunch of play events up for a single effect, we only care about the last one: if the driver/device had kept up, they'd all be played.  Since it didn't, they're gone (expired) and the program wants us to do something else (even if it's the same thing).

If we update and play and stop and do all sorts of things, and then go and DELETE the effect, then all those updates and plays and stops are pointless.  Don't bother with them.

What this all boils down to is that we:
  1. Keep the queue squashed to the almost minimum possible size (I said there were improvements to make...)
  2. Send significantly fewer events (faster queue processing, since we're not waiting on synchronous USB calls to complete all the time)
  3. Send only the most recent events
  4. Maintain a sensible, current state, so that we're not playing old effects that have since been updated.
Effectively, nothing is lost by doing the above.  We're not skipping anything that matters.  We are only skipping requests that don't matter, because they were replaced with newer requests.  As long as the assumption holds that the wheel itself is simple-minded, all this works actually quite well.

Ideally, the Logitech FF subsystem in the kernel would be better off maintaining a FULL STATE for the device (i.e. the effects), such that all it cares about sending during its workqueue job is the delta of its old state to its current state.  I might eventually get around to doing that.  This would finally squash the last minor bug: the create/destroy queue exploder bug.

This last bug is where a program (I'm looking at you, ETS2) creates and destroys effects super-fast.  Ignore the pointlessness of this, and just deal with the fact that IT HAPPENS.  In the current patch, creates and deletes only really affect updates and play/stops.  This means the queue can load up with creates and deletes.  The good news is that these are very quickly processed, such that instead of waiting five minutes (no joke) for it to catch up, you might have to wait all of about 5 seconds, by which point the game will have finished loading into the level and you'll be processing FF events in realtime.

Anyway, enough exposition.  Here's the patch.  I'm not going into the gritty details of applying it - that's left as an exercise to the reader.  After all, if you've kept up this far, you probably know how to patch and build a kernel.  NOTE: it's actually not terrible to just build the module itself and overwrite your existing kernel module, as long as all the version numbers match up.  This is how I did early testing.  Just be aware that the module will be lost if you upgrade your kernel...at least until I get this stupid thing submitted.


--- /a/hid-logitech-hidpp.c	2020-05-17 02:17:49.005405172 -0400
+++ /b/hid-logitech-hidpp.c	2020-05-17 02:17:17.398901709 -0400
@@ -26,6 +26,7 @@
 #include <linux/workqueue.h>
 #include <linux/atomic.h>
 #include <linux/fixp-arith.h>
+#include <linux/mutex.h>
 #include <asm/unaligned.h>
 #include "usbhid/usbhid.h"
 #include "hid-ids.h"
@@ -34,12 +35,12 @@
 MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
 MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
 
-static bool disable_raw_mode;
+static bool disable_raw_mode = false;
 module_param(disable_raw_mode, bool, 0644);
 MODULE_PARM_DESC(disable_raw_mode,
 	"Disable Raw mode reporting for touchpads and keep firmware gestures.");
 
-static bool disable_tap_to_click;
+static bool disable_tap_to_click = false;
 module_param(disable_tap_to_click, bool, 0644);
 MODULE_PARM_DESC(disable_tap_to_click,
 	"Disable Tap-To-Click mode reporting for touchpads (only on the K400 currently).");
@@ -157,7 +158,7 @@
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	int fields_count, ret;
 
-	hidpp = hid_get_drvdata(hdev);
+//	hidpp = hid_get_drvdata(hdev);
 
 	switch (hidpp_report->report_id) {
 	case REPORT_ID_HIDPP_SHORT:
@@ -815,6 +816,14 @@
 #define HIDPP_FF_EFFECTID_NONE		-1
 #define HIDPP_FF_EFFECTID_AUTOCENTER	-2
 
+#define HIDPP_FF_EFFECT_STATUS_PLAY_PENDING	0x01
+#define HIDPP_FF_EFFECT_STATUS_UPDATE_PENDING	0x02
+#define HIDPP_FF_EFFECT_STATUS_STOP_PENDING	0x03
+#define HIDPP_FF_EFFECT_STATUS_STOPPED		0x04
+#define HIDPP_FF_EFFECT_STATUS_ASSIGN_PENDING	0x05
+#define HIDPP_FF_EFFECT_STATUS_PLAYING		0x06
+#define HIDPP_FF_EFFECT_STATUS_DESTROYING	0x07
+
 #define HIDPP_FF_MAX_PARAMS	20
 #define HIDPP_FF_RESERVED_SLOTS	1
 
@@ -827,10 +836,19 @@
 	u8 slot_autocenter;
 	u8 num_effects;
 	int *effect_ids;
+	struct mutex post_mutex;
+	struct mutex update_mutex;
+	unsigned long *effect_status;
+	struct hidpp_ff_effect_update_data *effect_updates;
 	struct workqueue_struct *wq;
 	atomic_t workqueue_size;
 };
 
+struct hidpp_ff_effect_update_data {
+	u8 params[HIDPP_FF_MAX_PARAMS];
+	u8 size;
+};
+
 struct hidpp_ff_work_data {
 	struct work_struct work;
 	struct hidpp_ff_private_data *data;
@@ -840,7 +858,7 @@
 	u8 size;
 };
 
-static const signed short hiddpp_ff_effects[] = {
+static const signed short hidpp_ff_effects[] = {
 	FF_CONSTANT,
 	FF_PERIODIC,
 	FF_SINE,
@@ -855,7 +873,7 @@
 	-1
 };
 
-static const signed short hiddpp_ff_effects_v2[] = {
+static const signed short hidpp_ff_effects_v2[] = {
 	FF_RAMP,
 	FF_FRICTION,
 	FF_INERTIA,
@@ -876,7 +894,17 @@
 	"inertia"
 };
 
+static const char *HIDPP_FF_PERIODIC_NAMES[] = {
+	"invalid",
+	"sine",
+	"square",
+	"triangle",
+	"sawtooth-up",
+	"sawtooth-down"
+};
+	
 
+/*
 static u8 hidpp_ff_find_effect(struct hidpp_ff_private_data *data, int effect_id)
 {
 	int i;
@@ -887,6 +915,7 @@
 
 	return 0;
 }
+*/
 
 static void hidpp_ff_work_handler(struct work_struct *w)
 {
@@ -896,61 +925,189 @@
 	u8 slot;
 	int ret;
 
+	/* If the command is DOWNLOAD_EFFECT, we are going to use the data
+	 * from the internal effect_update array, since it represents the
+	 * most recently received effect update for this effect. 
+	 * DOES NOT APPLY TO RESERVED EFFECTS!!
+	 *
+	 * If it is an effect STATE UPDATE, we set the appropriate parameter
+	 * value and clear the playback update flags.
+	 *
+	 * If the effect is scheduled to be DESTROYed, then we can skip
+	 * playing, stopping, or updating the effect altogether.  
+	 */
+	mutex_lock(&data->update_mutex);
+	switch (wd->command) {
+	case HIDPP_FF_DOWNLOAD_EFFECT:
+		if (wd->effect_id >= 0) {
+			wd->size = data->effect_updates[wd->effect_id].size;
+			memcpy(wd->params, data->effect_updates[wd->effect_id].params, wd->size);
+	
+			clear_bit(HIDPP_FF_EFFECT_STATUS_UPDATE_PENDING, &data->effect_status[wd->effect_id]);
+		}
+		break;
+	case HIDPP_FF_SET_EFFECT_STATE:
+		/* Technically only ONE of these flags should be active... */
+		if (test_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[wd->effect_id]) &&
+		   test_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[wd->effect_id])) {
+			hid_warn(data->hidpp->hid_dev, "Both play and stop request flags found for effect id %d - stopping effect! (This might not be what you want.)\n", wd->effect_id);
+		
+			clear_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[wd->effect_id]);
+		}
+
+		if (test_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[wd->effect_id])) {
+			wd->params[1] = HIDPP_FF_EFFECT_STATE_PLAY;
+			set_bit(HIDPP_FF_EFFECT_STATUS_PLAYING, &data->effect_status[wd->effect_id]);
+			clear_bit(HIDPP_FF_EFFECT_STATUS_STOPPED, &data->effect_status[wd->effect_id]);
+		} else if (test_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[wd->effect_id])) {
+			wd->params[1] = HIDPP_FF_EFFECT_STATE_STOP;
+			set_bit(HIDPP_FF_EFFECT_STATUS_STOPPED, &data->effect_status[wd->effect_id]);
+			clear_bit(HIDPP_FF_EFFECT_STATUS_PLAYING, &data->effect_status[wd->effect_id]);
+		}
+
+		clear_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[wd->effect_id]);
+		clear_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[wd->effect_id]);
+
+		break;
+	}		
+
+	/* Check to see if we should even do what we've been told to do... */
+	if (wd->effect_id >= 0 && test_bit(HIDPP_FF_EFFECT_STATUS_DESTROYING, &data->effect_status[wd->effect_id]) &&
+	    (wd->command == HIDPP_FF_DOWNLOAD_EFFECT || wd->command == HIDPP_FF_SET_EFFECT_STATE)) {
+		/* We're about to destroy this effect.  Skip whatever this is. */
+		mutex_unlock(&data->update_mutex);
+		goto out;
+	}
+
+	mutex_unlock(&data->update_mutex);
+
+
 	/* add slot number if needed */
+	/* ff_core provides a zero-based effect ID to us and to the caller.
+	 * It takes care of the "effect assignment," since we are in another
+	 * thread and the original ioctl call already returned to caller.
+	 * We have one effect reserved and it's in its own slot: autocenter.
+	 * Everything else simply needs to map between the caller's ID and the
+	 * ID that the device supplies us.
+	 */
 	switch (wd->effect_id) {
 	case HIDPP_FF_EFFECTID_AUTOCENTER:
+		//dbg_hid("work_handler slot_autocenter selected: %d\n", data->slot_autocenter);
 		wd->params[0] = data->slot_autocenter;
 		break;
 	case HIDPP_FF_EFFECTID_NONE:
 		/* leave slot as zero */
+		//dbg_hid("work_handler no slot selected\n");
 		break;
 	default:
 		/* find current slot for effect */
-		wd->params[0] = hidpp_ff_find_effect(data, wd->effect_id);
+		wd->params[0] = data->effect_ids[wd->effect_id];
+		//dbg_hid("work_handler get slot for effect_id=%d -> %d\n", wd->effect_id, wd->params[0]);
 		break;
 	}
 
+
 	/* send command and wait for reply */
 	ret = hidpp_send_fap_command_sync(data->hidpp, data->feature_index,
 		wd->command, wd->params, wd->size, &response);
 
 	if (ret) {
 		hid_err(data->hidpp->hid_dev, "Failed to send command to device!\n");
+		switch (wd->command) {
+		case HIDPP_FF_DOWNLOAD_EFFECT:
+			hid_err(data->hidpp->hid_dev, "--- failed command: DOWNLOAD EFFECT - id=%d\n", wd->effect_id); 
+			break;
+		case HIDPP_FF_DESTROY_EFFECT:
+			hid_err(data->hidpp->hid_dev, "--- failed command: DESTROY EFFECT - id=%d\n", wd->effect_id); 
+			break;
+		case HIDPP_FF_SET_GLOBAL_GAINS:
+			hid_err(data->hidpp->hid_dev, "--- failed command: SET GLOBAL GAIN - id=%d\n", wd->effect_id); 
+			break;
+		case HIDPP_FF_SET_APERTURE:
+			hid_err(data->hidpp->hid_dev, "--- failed command: SET APERTURE - id=%d\n", wd->effect_id); 
+			break;
+		case HIDPP_FF_SET_EFFECT_STATE:
+			hid_err(data->hidpp->hid_dev, "--- failed command: SET EFFECT STATE - id=%d\n", wd->effect_id); 
+			break;
+		default:
+			hid_err(data->hidpp->hid_dev, "--- failed command: OTHER... - id=%d\n", wd->effect_id);
+		}
 		goto out;
 	}
 
 	/* parse return data */
+	mutex_lock(&data->update_mutex);
 	switch (wd->command) {
 	case HIDPP_FF_DOWNLOAD_EFFECT:
+		dbg_hid("work_handler: download effect id=%d\n", wd->effect_id);
+		//hid_warn(data->hidpp->hid_dev, "work handler: download effect id %d\n", wd->effect_id);
 		slot = response.fap.params[0];
-		if (slot > 0 && slot <= data->num_effects) {
-			if (wd->effect_id >= 0)
-				/* regular effect uploaded */
-				data->effect_ids[slot-1] = wd->effect_id;
-			else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
-				/* autocenter spring uploaded */
+		//dbg_hid("work_handler response slot: %d\n", slot);
+		/* The device returns a non-zero effect ID.  Store it in the map
+		 * unless we know we've just assigned the AUTOCENTER effect.
+		 */
+		if (slot > 0) {
+			switch (wd->effect_id) {
+			case HIDPP_FF_EFFECTID_AUTOCENTER:
+                               /* autocenter spring uploaded */
+				//dbg_hid("assigning autocenter slot # %d\n", slot);
 				data->slot_autocenter = slot;
+				break;
+			default:
+				//dbg_hid("assigning/updating effect-id %d slot # %d (array index %d)\n", wd->effect_id, slot, wd->effect_id);
+				/* regular effect uploaded */
+				data->effect_ids[wd->effect_id] = slot;
+				break;
+			}
 		}
+
+		clear_bit(HIDPP_FF_EFFECT_STATUS_ASSIGN_PENDING, &data->effect_status[wd->effect_id]);
+
 		break;
 	case HIDPP_FF_DESTROY_EFFECT:
-		if (wd->effect_id >= 0)
-			/* regular effect destroyed */
-			data->effect_ids[wd->params[0]-1] = -1;
-		else if (wd->effect_id >= HIDPP_FF_EFFECTID_AUTOCENTER)
+		dbg_hid("work_handler: destroy effect id=%d\n", wd->effect_id);
+		//hid_warn(data->hidpp->hid_dev, "work handler: destroy effect id %d\n", wd->effect_id);
+		switch (wd->effect_id) {
+		case HIDPP_FF_EFFECTID_AUTOCENTER:
 			/* autocenter spring destoyed */
 			data->slot_autocenter = 0;
+			break;
+		default:
+			/* regular effect destroyed - Erase the mapping. */
+			data->effect_ids[wd->effect_id] = 0;
+
+			/* Do not destroy status bits, since there may be outstanding commands later in the queue. */
+			/* Instead, just set/clear the appropriate bits. */
+			clear_bit(HIDPP_FF_EFFECT_STATUS_DESTROYING, &data->effect_status[wd->effect_id]);
+			clear_bit(HIDPP_FF_EFFECT_STATUS_PLAYING, &data->effect_status[wd->effect_id]);
+			set_bit(HIDPP_FF_EFFECT_STATUS_STOPPED, &data->effect_status[wd->effect_id]);
+			break;
+		}
 		break;
 	case HIDPP_FF_SET_GLOBAL_GAINS:
+		dbg_hid("work_handler: set global gains\n");
 		data->gain = (wd->params[0] << 8) + wd->params[1];
 		break;
 	case HIDPP_FF_SET_APERTURE:
+		dbg_hid("work_handler: set aperture\n");
 		data->range = (wd->params[0] << 8) + wd->params[1];
 		break;
+	case HIDPP_FF_SET_EFFECT_STATE:
+		/* always clear the play_pending bit, even if it isn't set.
+		 * We only care that an effect is either been manually stopped, 
+		 * or that it has executed its play command. */ 
+		dbg_hid("work_handler: set effect state id=%d\n", wd->effect_id);
+		//hid_warn(data->hidpp->hid_dev, "work handler: play/stop effect id %d\n", wd->effect_id);
+		//clear_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[wd->effect_id]);
+		break;
 	default:
+		dbg_hid("work_handler: other\n");
 		/* no action needed */
 		break;
 	}
 
+	mutex_unlock(&data->update_mutex);
+
 out:
 	atomic_dec(&data->workqueue_size);
 	kfree(wd);
@@ -973,13 +1130,13 @@
 	memcpy(wd->params, params, size);
 
 	atomic_inc(&data->workqueue_size);
-	queue_work(data->wq, &wd->work);
-
 	/* warn about excessive queue size */
 	s = atomic_read(&data->workqueue_size);
 	if (s >= 20 && s % 20 == 0)
 		hid_warn(data->hidpp->hid_dev, "Force feedback command queue contains %d commands, causing substantial delays!", s);
 
+	queue_work(data->wq, &wd->work);
+		
 	return 0;
 }
 
@@ -990,6 +1147,11 @@
 	u8 size;
 	int force;
 
+	if (effect->id < 0 || effect->id >= data->num_effects)
+		return -EINVAL;
+
+	//hid_warn(data->hidpp->hid_dev, "update effect id %d\n", effect->id);
+
 	/* set common parameters */
 	params[2] = effect->replay.length >> 8;
 	params[3] = effect->replay.length & 255;
@@ -1009,14 +1171,21 @@
 		params[12] = effect->u.constant.envelope.fade_length >> 8;
 		params[13] = effect->u.constant.envelope.fade_length & 255;
 		size = 14;
-		dbg_hid("Uploading constant force level=%d in dir %d = %d\n",
+		dbg_hid("Uploading id [%d] CONSTANT force\n", effect->id);
+		dbg_hid("** [%d] CONSTANT level=%d in dir %d = %d\n",
+				effect->id,
 				effect->u.constant.level,
 				effect->direction, force);
-		dbg_hid("          envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+		dbg_hid("** [%d] CONSTANT envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+				effect->id,
 				effect->u.constant.envelope.attack_level,
 				effect->u.constant.envelope.attack_length,
 				effect->u.constant.envelope.fade_level,
 				effect->u.constant.envelope.fade_length);
+		dbg_hid("** [%d] CONSTANT replay duration=%d ms, delay=%d ms\n", 
+				effect->id,
+				effect->replay.length,
+				effect->replay.delay);
 		break;
 	case FF_PERIODIC:
 	{
@@ -1056,16 +1225,23 @@
 		params[18] = effect->u.periodic.envelope.fade_length >> 8;
 		params[19] = effect->u.periodic.envelope.fade_length & 255;
 		size = 20;
-		dbg_hid("Uploading periodic force mag=%d/dir=%d, offset=%d, period=%d ms, phase=%d\n",
+		dbg_hid("Uploading id [%d] PERIODIC <%s> force\n", effect->id, HIDPP_FF_PERIODIC_NAMES[params[1]]);
+		dbg_hid("** [%d] PERIODIC mag=%d/dir=%d, offset=%d, period=%d ms, phase=%d\n",
+				effect->id,
 				effect->u.periodic.magnitude, effect->direction,
 				effect->u.periodic.offset,
 				effect->u.periodic.period,
 				effect->u.periodic.phase);
-		dbg_hid("          envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+		dbg_hid("** [%d] PERIODIC envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+				effect->id,
 				effect->u.periodic.envelope.attack_level,
 				effect->u.periodic.envelope.attack_length,
 				effect->u.periodic.envelope.fade_level,
 				effect->u.periodic.envelope.fade_length);
+		dbg_hid("** [%d] PERIODIC replay duration=%d ms, delay=%d ms\n", 
+				effect->id,
+				effect->replay.length,
+				effect->replay.delay);
 		break;
 	}
 	case FF_RAMP:
@@ -1083,15 +1259,22 @@
 		params[14] = effect->u.ramp.envelope.fade_length >> 8;
 		params[15] = effect->u.ramp.envelope.fade_length & 255;
 		size = 16;
-		dbg_hid("Uploading ramp force level=%d -> %d in dir %d = %d\n",
+		dbg_hid("Uploading id [%d] RAMP force\n", effect->id);
+		dbg_hid("** [%d] RAMP level=%d -> %d in dir %d = %d\n",
+				effect->id,
 				effect->u.ramp.start_level,
 				effect->u.ramp.end_level,
 				effect->direction, force);
-		dbg_hid("          envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+		dbg_hid("** [%d] RAMP envelope attack=(%d, %d ms) fade=(%d, %d ms)\n",
+				effect->id,
 				effect->u.ramp.envelope.attack_level,
 				effect->u.ramp.envelope.attack_length,
 				effect->u.ramp.envelope.fade_level,
 				effect->u.ramp.envelope.fade_length);
+		dbg_hid("** [%d] RAMP replay duration=%d ms, delay=%d ms\n", 
+				effect->id,
+				effect->replay.length,
+				effect->replay.delay);
 		break;
 	case FF_FRICTION:
 	case FF_INERTIA:
@@ -1111,21 +1294,59 @@
 		params[16] = effect->u.condition[0].right_saturation >> 9;
 		params[17] = (effect->u.condition[0].right_saturation >> 1) & 255;
 		size = 18;
-		dbg_hid("Uploading %s force left coeff=%d, left sat=%d, right coeff=%d, right sat=%d\n",
-				HIDPP_FF_CONDITION_NAMES[effect->type - FF_SPRING],
+		dbg_hid("Uploading id [%d] CONDITION <%s> force\n", 
+				effect->id,
+				HIDPP_FF_CONDITION_NAMES[effect->type - FF_SPRING]);
+		dbg_hid("** [%d] CONDITION left coeff=%d, left sat=%d, right coeff=%d, right sat=%d\n",
+				effect->id,
 				effect->u.condition[0].left_coeff,
 				effect->u.condition[0].left_saturation,
 				effect->u.condition[0].right_coeff,
 				effect->u.condition[0].right_saturation);
-		dbg_hid("          deadband=%d, center=%d\n",
+		dbg_hid("** [%d] CONDITION deadband=%d, center=%d\n",
+				effect->id,
 				effect->u.condition[0].deadband,
 				effect->u.condition[0].center);
+		dbg_hid("** [%d] CONDITION replay duration=%d ms, delay=%d ms\n", 
+				effect->id,
+				effect->replay.length,
+				effect->replay.delay);
 		break;
 	default:
 		hid_err(data->hidpp->hid_dev, "Unexpected force type %i!\n", effect->type);
 		return -EINVAL;
 	}
 
+	/* copy data into effect_updates array. */
+	mutex_lock(&data->update_mutex);
+
+	data->effect_updates[effect->id].size = size;
+	/* memcpy here is not as efficient as it could be, but it's also vastly safer since
+	 * the above code might return -EINVAL at multiple places. */
+	memcpy(data->effect_updates[effect->id].params, params, size);
+
+	/* If there is an update already pending, we're done and can simply return. */
+	/* Note that an ASSIGN can only be true if UPDATE is true. */
+	if (test_bit(HIDPP_FF_EFFECT_STATUS_UPDATE_PENDING, &data->effect_status[effect->id])) {
+		dbg_hid("** [%d] overwriting outstanding request\n", effect->id);
+		//hid_warn(data->hidpp->hid_dev, "Rapid update of FF effect: overwriting outstanding request for effect id %d\n", effect->id);
+		mutex_unlock(&data->update_mutex);
+		return 0;
+	}
+
+	/* Set the update bit and push the update onto the workqueue. */
+	set_bit(HIDPP_FF_EFFECT_STATUS_UPDATE_PENDING, &data->effect_status[effect->id]);
+
+	/* This will be a new slot assignment */
+	if (data->effect_ids[effect->id] == 0)
+		set_bit(HIDPP_FF_EFFECT_STATUS_ASSIGN_PENDING, &data->effect_status[effect->id]);
+
+	mutex_unlock(&data->update_mutex);
+
+	/* Don't copy ram again. */
+	size = 0;
+	dbg_hid("** [%d] posting new request\n", effect->id);
+
 	return hidpp_ff_queue_work(data, effect->id, HIDPP_FF_DOWNLOAD_EFFECT, params, size);
 }
 
@@ -1133,12 +1354,58 @@
 {
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 params[2];
+	bool play_pending, stop_pending, playing, stopped;
+	bool queue_update;
+
+	if (effect_id < 0 || effect_id >= data->num_effects)
+		return -EINVAL;
+
+	dbg_hid("ST%s playback of effect [%d].\n", value?"ART":"OP", effect_id);
+	//hid_warn(data->hidpp->hid_dev, "ST%s playback of effect [%d].\n", value?"ART":"OP", effect_id);
+
+	/* don't cue work that is already done. */
+	/* If we queued a play request and it hasn't been processed yet, don't queue another.
+	 * Since we can't tell when playing stops (if it stops itself), we will always honor
+	 * play requests as long as they don't stack up.
+	 * Stop requests are always considered instanteous and final.
+	 */
+	mutex_lock(&data->update_mutex);
+	play_pending = test_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[effect_id]);
+	playing      = test_bit(HIDPP_FF_EFFECT_STATUS_PLAYING, &data->effect_status[effect_id]);
+	stop_pending = test_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[effect_id]);
+	stopped      = test_bit(HIDPP_FF_EFFECT_STATUS_STOPPED, &data->effect_status[effect_id]);
+
+	if ((value && (play_pending || playing)) || (!value && (stop_pending || stopped))) {
+		dbg_hid("Playback of effect [%d] already ST%sED.\n", effect_id, value?"ART":"OPP");
+		mutex_unlock(&data->update_mutex);
+ 		return 0;
+	}
+
+	/* If we already have a playback update pending, don't queue up another update.
+	 * Instead, just update the necessary flag. 
+	 */
+	if (play_pending || stop_pending) {
+		queue_update = false;
+	} else {
+		queue_update = true;
+	}
 
-	params[1] = value ? HIDPP_FF_EFFECT_STATE_PLAY : HIDPP_FF_EFFECT_STATE_STOP;
+//	params[1] = value ? HIDPP_FF_EFFECT_STATE_PLAY : HIDPP_FF_EFFECT_STATE_STOP;
 
-	dbg_hid("St%sing playback of effect %d.\n", value?"art":"opp", effect_id);
+	/* set the appropriate flags */
+	if (value) {
+		set_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[effect_id]);
+		clear_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[effect_id]);
+	} else {
+		clear_bit(HIDPP_FF_EFFECT_STATUS_PLAY_PENDING, &data->effect_status[effect_id]);
+		set_bit(HIDPP_FF_EFFECT_STATUS_STOP_PENDING, &data->effect_status[effect_id]);
+	}
+	mutex_unlock(&data->update_mutex);
 
-	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
+	if (queue_update)
+		return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_SET_EFFECT_STATE, params, ARRAY_SIZE(params));
+	
+	return 0;
 }
 
 static int hidpp_ff_erase_effect(struct input_dev *dev, int effect_id)
@@ -1146,7 +1413,29 @@
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 slot = 0;
 
-	dbg_hid("Erasing effect %d.\n", effect_id);
+	if (effect_id == HIDPP_FF_EFFECTID_AUTOCENTER) {
+		/* We are requesting the autocenter be destroyed... for now, just warn. */
+		hid_err(data->hidpp->hid_dev, "WARNING: FEATURE NOT IMPLEMENTED - destroy AUTOCENTER effect\n");
+		return -EINVAL;
+	}
+
+	if (effect_id < 0 || effect_id >= data->num_effects)
+		return -EINVAL;
+
+        mutex_lock(&data->update_mutex);
+	/* Check that we either uploaded an effect that hasn't finished uploading yet,
+	 * or we have not defined this effect. */
+	if ((!data->effect_ids[effect_id] && !test_bit(HIDPP_FF_EFFECT_STATUS_ASSIGN_PENDING, &data->effect_status[effect_id])) ||
+	     test_bit(HIDPP_FF_EFFECT_STATUS_DESTROYING, &data->effect_status[effect_id])) {
+        	mutex_unlock(&data->update_mutex);
+		return 0;
+	}
+
+	set_bit(HIDPP_FF_EFFECT_STATUS_DESTROYING, &data->effect_status[effect_id]);
+        mutex_unlock(&data->update_mutex);
+
+	/* It's either defined or about to be.  Issue its destruction. */
+	dbg_hid("ERASE effect [%d].\n", effect_id);
 
 	return hidpp_ff_queue_work(data, effect_id, HIDPP_FF_DESTROY_EFFECT, &slot, 1);
 }
@@ -1156,7 +1445,7 @@
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 params[18];
 
-	dbg_hid("Setting autocenter to %d.\n", magnitude);
+	dbg_hid("SET AUTOCENTER to magnitude %d.\n", magnitude);
 
 	/* start a standard spring effect */
 	params[1] = HIDPP_FF_EFFECT_SPRING | HIDPP_FF_EFFECT_AUTOSTART;
@@ -1178,7 +1467,7 @@
 	struct hidpp_ff_private_data *data = dev->ff->private;
 	u8 params[4];
 
-	dbg_hid("Setting gain to %d.\n", gain);
+	dbg_hid("SET GAIN to %d.\n", gain);
 
 	params[0] = gain >> 8;
 	params[1] = gain & 255;
@@ -1224,6 +1513,11 @@
 	struct hidpp_ff_private_data *data = ff->private;
 
 	kfree(data->effect_ids);
+        kfree(data->effect_updates);
+        kfree(data->effect_status);
+
+	mutex_destroy(&data->update_mutex);
+	mutex_destroy(&data->post_mutex);
 }
 
 static int hidpp_ff_init(struct hidpp_device *hidpp, u8 feature_index)
@@ -1248,11 +1542,11 @@
 	version = bcdDevice & 255;
 
 	/* Set supported force feedback capabilities */
-	for (j = 0; hiddpp_ff_effects[j] >= 0; j++)
-		set_bit(hiddpp_ff_effects[j], dev->ffbit);
+	for (j = 0; hidpp_ff_effects[j] >= 0; j++)
+		set_bit(hidpp_ff_effects[j], dev->ffbit);
 	if (version > 1)
-		for (j = 0; hiddpp_ff_effects_v2[j] >= 0; j++)
-			set_bit(hiddpp_ff_effects_v2[j], dev->ffbit);
+		for (j = 0; hidpp_ff_effects_v2[j] >= 0; j++)
+			set_bit(hidpp_ff_effects_v2[j], dev->ffbit);
 
 	/* Read number of slots available in device */
 	error = hidpp_send_fap_command_sync(hidpp, feature_index,
@@ -1287,8 +1581,25 @@
 	data->version = version;
 	data->slot_autocenter = 0;
 	data->num_effects = num_slots;
-	for (j = 0; j < num_slots; j++)
-		data->effect_ids[j] = -1;
+
+        data->effect_updates = kcalloc(num_slots, sizeof(*(data->effect_updates)), GFP_KERNEL);
+        if (!data->effect_updates) {
+		kfree(data->effect_ids);
+                kfree(data);
+                return -ENOMEM;
+        }
+
+
+        data->effect_status = kcalloc(num_slots, sizeof(unsigned long), GFP_KERNEL);
+        if (!data->effect_status) {
+                kfree(data->effect_updates);
+                kfree(data->effect_ids);
+                kfree(data);
+                return -ENOMEM;
+        }
+
+	mutex_init(&data->post_mutex);
+	mutex_init(&data->update_mutex);
 
 	ff = dev->ff;
 	ff->private = data;